Skip to content
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

Hint about memory usage in README #17

Closed
anatolsommer opened this issue Jan 25, 2016 · 11 comments
Closed

Hint about memory usage in README #17

anatolsommer opened this issue Jan 25, 2016 · 11 comments

Comments

@anatolsommer
Copy link

It would be great if there was a hint that constantly calling pusage.stat (or fs.readFile/createReadStream in general) is filling up memory pretty fast if you don't call gc manually every few seconds. This might save a lot of people a lot of time.

@soyuka
Copy link
Owner

soyuka commented Jan 25, 2016

In fact we keep a history to get more accurate data on linux. To clean up this you have to use the unmonitor function. I don't think that fs.readFile is the issue here.

@anatolsommer
Copy link
Author

It is. I spent more than 5 hours in tracking down that problem yesterday.

var fs=require('fs'), noop=function() {};

(function readFile() {
  for (var i=0; i<500; i++) {
    fs.readFile(__filename, noop);
  }
  //gc();
  setTimeout(readFile, 50);
})();

Result: 60MB RSS within a few seconds - if you uncomment gc() memory usage constantly stays below 20MB RSS. Tested on different VMs and linux distributions, surprisingly there seems to be no problem on windows.

@soyuka
Copy link
Owner

soyuka commented Jan 25, 2016

50ms is very low. I can improve that and will do, thanks.

@anatolsommer
Copy link
Author

I started to inspect the problem, when my script calling pusage.stat every 1000ms used 80MB RSS after a few hours, so I can confirm this also affects real life situations. Thank you too.

@soyuka
Copy link
Owner

soyuka commented Jan 26, 2016

So, I tried to keep the fd cursor in memory to avoid (re)opening the file every time but it does not make any difference and fs.read() still uses memory and the gc() behavior is the same (commit).

May I ask what node version do you use? I launched my last test on v5.5 without improvements :|

@anatolsommer
Copy link
Author

I used v0.12, v4.2.6 and v5.5 in my tests and saw no real difference between these versions.

@soyuka
Copy link
Owner

soyuka commented Jan 26, 2016

Thanks, I guess I'll just add a warning to the readme as I can't find any fix :|. Do you think the memory consumption is worth opening an issue on nodejs/node? Taking the fact that using gc() makes it behave normally, I'm wondering.

@anatolsommer
Copy link
Author

Someone did report the bug, but they closed it since gc() solves the problem and it's not a real memory leak (at some point memory stops filling up). Sorry, can't find the link right now...

@soyuka soyuka closed this as completed in cd988d4 Jan 26, 2016
soyuka added a commit that referenced this issue Jan 26, 2016
@ghost
Copy link

ghost commented Apr 25, 2016

This comment is confusing since it uses "leaking". As @anatolsommer stated above, this is not a memory leak, which wouldn't be corrected by calling gc().

@retrohacker
Copy link

Is this a leak? Is the disclaimer necessary?

Won't Node.js automatically run the gc when it needs to? Running the gc() call every 100ms is extremely expensive.

soyuka added a commit that referenced this issue Aug 23, 2017
@soyuka
Copy link
Owner

soyuka commented Aug 23, 2017

"leak" is a bit strong indeed, changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants