-
-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
An unusually large amount of memory usage #19
Comments
Perhaps linked to #12 |
To be honest, I would be open to migrating TaskGroup to a pre-processor that actually cares about performance. CoffeeScript no longer cares about it at all, which is really sad. Perhaps CoffeeScript Redux is an option, otherwise we could look at TypeScript, or maybe just native javascript (not that it is actually a good language - unlike compared to 5 years ago) / ref https://developers.google.com/v8/experiments @pflannery I've started a discussion about this here: https://discuss.bevry.me/t/move-from-coffeescript-to-es6/30/1 |
Seeing as the topic has gone and the ssl is telling me that it's not valid. I will just reply here for now :) I agree, I think moving to native ES5\ES6 will give us more control over what code we use. I'm all for it. I've been using v8-profiler to take snapshots of the test code I posted above and below you see that a Task instance is around 8K and the _events property generated by the EventEmitter is adding a 5-6kb per Task. So I'm wonder if its worth replacing the event emitter for something that is much lighter in weight? |
https://discuss.bevry.me/t/move-from-coffeescript-to-es6/30 is the new link :-) |
Just want to correct myself slightly, EventEmitter isn't adding 5-6k per task, instead it just seems that its referencing 5-6k. |
@pflannery can you do a screencast sometime of how you come up with that profile information - I've spent ages trying to figure it out and no luck! |
Here's the code for generating the heap snapshot.. var fs = require('fs');
var profiler = require('v8-profiler');
function saveSnapshot(fileName) {
var buffer = '';
var snapshot = profiler.takeSnapshot("test");
var t = snapshot.serialize(
function iterator(data, length) {
buffer += data;
},function complete(){
fs.writeFileSync(fileName, buffer)
}
);
}
var TaskGroup = require("./taskgroup").TaskGroup;
var testArray = Array(10000).join().split(',');
var grp = TaskGroup.create();
testArray.forEach(function(value, index) {
grp.addTask(function() {
// console.log("task"+index);
});
});
grp.on("completed", function(){
saveSnapshot("completed.heapsnapshot")
});
grp.run(); |
also here is a Memory Management Masterclass video on https://www.youtube.com/watch?v=LaxbdIyBkL0 |
I've created a gist with javascript that generates the cpu profile and ensures the deopt reasons will render in latest chrome - https://gist.github.com/pflannery/8e38a06a844a7fc362dc setup instructions
|
Sweet. I'll give it a go (watching the talk right now). I'm wondering if it is best to just do the performance tracking inside the browser with a browserified/webpack'd build of taskgroup. Your thoughts? |
I've never used browserified or webpack so don't know what the outcome would be. |
@balupton It wouldn;t let me post post than three times on the discuss thread and it wont let me add any more links in the post either so I've replied here I've been analysing the heap generated from running the my test and i've seen that commenting out the following lines takes the heap snapshot from 114MB down to 31MB. (the snapshot was taken once during the taskgroup completed event) Lines here and Lines here So from that I think this confirms that the EventEmitter (taking up 53MB) is a big part of this problem and it also seems that the Map copying (taking up 14MB) is adding to it too. |
Interesting. A few notes: Commenting out those events means that the taskgroup of 31MB doesn't work... Perhaps the downsize of the commented out map options are actually because it disables domains. My other thought is that it the completed event will be too early, as the normal garbage collection wouldn't have kicked in yet, it will have to be once the taskgroup has completed and the scope of the taskgroup instance has been forgotten. I'll do some evaluating of this today. Keep up the good work. |
@pflannery think you came across these rate limits: https://www.dropbox.com/s/sltz6p9gavq64vz/Screenshot%202015-03-24%2008.25.01.png?dl=0 Tomorrow it won't be a problem :-) |
@pflannery I've pushed some profiling things up to the
To generate the V8 profile report. However, you can also run:
Then serve the |
Chrome's TaskGroup (CoffeeScript version) profile (13 seconds): https://www.dropbox.com/s/lnh01tehmtsv2cb/taskgroup-coffeescript.cpuprofile?dl=0 Conclusions from evaluating the running of tasks:
|
Chrome's TaskGroup (ES6 babel version) profile (8 seconds): https://www.dropbox.com/s/alsdwj0u97sm09c/taskgroup-es6-babel.cpuprofile?dl=0 |
Okay I was able to get it down to 3.5 seconds by removing maps and disabling nested events by default. Removing the next tick stuff actually makes it go way slower, as the stack continues to grow. @pflannery any idea how to evaluate the heap snapshots to tell if it has cleaned up properly, I have no clue how I am meant to read those snapshots. |
Firstly what I do is replace babel's _applyConstructor code (at the very top of TaskGroup output) so we can see the Task class in the heap list, otherwise they get created as Object var _applyConstructor = function (Constructor, args) {
return new (Function.prototype.bind.apply(Constructor, [null].concat(args)));
}; The way I evaluate the heaps is to take three snapshots, one at the start, one just after run and then one when it's completed - like this example Also If you take a snapshot when destroy is emitted then that will show you what's left in memory after clean up. The three snapshot technique will go away soon as there is now a |
Working on Chrome Canary with #19 (comment) Will try your suggestion, hopefully it will make the snapshots make sense. |
Merged, but still getting segfaults on heaps (neither sync or async work). I don't get segfaults on profile writing sync or async (both work). |
The latest changes seems to of cleared this problem. I wonder how much of this was removing domains and how much was removing coffee? |
@pflannery domains are still enabled by default in node land (just disabled in browser land) |
@pflannery does the latest commits make this any better? Notably: 9edcd0f#commitcomment-10406680 |
Closed with v5.0.0 - https://github.com/bevry/taskgroup/blob/master/HISTORY.md#v500-2016-june-4 |
When creating 10000 small tasks I find that the process memory peeks around 170MB. When I run the same 10000 tasks with setImmediate it's doesn't go over 40MB.
So its seems that task group is generating an unusually large amount of memory overhead.
Here the code I was testing with.
I only tested on Windows 8.1 so not sure what a Linux box will output
The text was updated successfully, but these errors were encountered: