remy / nodemon

Monitor for any changes in your node.js application and automatically restart the server - perfect for development
http://nodemon.io/
MIT License
26.22k stars 1.72k forks source link

Use fs.watch on mac if ulimit has been raised #463

Closed kewisch closed 9 years ago

kewisch commented 9 years ago

I've read #342 which should fix detection for changes on mac, but I'd like to propose an intermediate solution, because I have the feeling using find is really killing my battery life.

Would it be acceptable to use the posix module to check if posix.getrlimit('nofile').soft has a sufficiently large value and if so, use fs.watch instead? My ubuntu instance seems to use 1024, maybe thats a good threshhold to check. Happy to whip up a patch if I get a yes.

remy commented 9 years ago

I'm wary that fs.watch on 50,000 files, that's 50,000 individual watches going on, is more expensive than find running every second...or maybe it's not. I'm not sure I have any evidence either way.

Can you try npm install -g nodemon@dev first - it's not the current code in master, but it does have find optimisations that correctly prune out the files you don't want to watch.

I'm open to PRs to nodemon to make it better, but often these core level changes don't take into account windows support, linux support, tests, edge cases and so on, so I'd rather tread carefully before you give up too much of your time for a PR that might not land.

At least try the @dev version of nodemon and let me know what you see, then go from there?

kewisch commented 9 years ago

Sure, I'll give the dev branch a try and see how it goes. This isn't a very solid test, but I removed the isMac check locally and while usually my fan starts churning fairly early when running our project over the evening, I haven't had the fan go on at all with fs.watch.

While I understand the fear that using fs.watch on so many files might not bring a performance benefit, I don't see why it should not be used on mac. You use fs.watch on other platforms where it is supported and it seems the only reason it was not done on mac is because of the default ulimit of 256. Are there other reasons that mac shouldn't use fs.watch?

The patch I am proposing would be but a few lines, replacing the isMac check with either a pure check for an ulimit > MAGIC_THRESHOLD (maybe 1024). If it makes you feel better w.r.t. edge cases, maybe we can go for a combined check if isMac and ulimit > 256. This would give a benefit for developers that have raised their ulimit and not affect other platforms.

remy commented 9 years ago

It's partly because I'm moving linux to the find based solution rather than fs.watch now too. Something in my gut suspects adding watches to 1000s of files is going to slower to boot compared to find. But if find is still hammering your cpu, then we've got a problem either way.

Give the edge version a try and let me know.

simonexmachina commented 9 years ago

sane is a really excellent file watching module. It's used in ember-cli and I can attest that it scales very well to large sets of files, doesn't smash the CPU and supports a variety of different watching mechanisms, including Facebook's watchman, which is the ultimate solution (albeit not the simplest) to this problem.

remy commented 9 years ago

See previous responses.

kewisch commented 9 years ago

The author of emcrisostomo/fswatch seems to have done some performance analysis between using a polling mechanism vs. a kernel notifier. Maybe @emcrisostomo could comment here? (Not sure github notifies people I haven't made contact with before but I hope so). I haven't tried dev yet, but I'll get back to you as soon as possible.

remy commented 9 years ago

How did you get on with dev?

remy commented 9 years ago

Okay, it's now live. I'm going to close this unless I get feedback that the find command is still causing significant issues.

simonexmachina commented 9 years ago

Sorry @remy, what's now live?

I can certainly attest that the find command is causing significant issues on our codebase, we don't use nodemon any more because it was hogging CPU and killing battery life.

remy commented 9 years ago

nodemon 1.3 is live with changes to the way it detects file changes.

You've said that find is causing CPU issues, but since you're not using nodemon, I think you meant was. No problem either way.

remy commented 9 years ago

nodemon 1.3 is live with changes to the way it detects file changes.

You've said that find is causing CPU issues, but since you're not using nodemon, I think you meant was. No problem either way.

emcrisostomo commented 9 years ago

Hi,

I'm stepping in on @kewisch's request (sorry for the belated answer but I had a week off). When I started adding monitor backends to fswatch I made some measurements in order to detect whether supporting a platform monitor would be useful. So far fswatch supports:

To support platforms where no other monitor is available, a poll-based monitor is available.

My conclusions were:

The baseline is: if you can use an available file change notification API, then use it. Even APIs such as kqueue, that need an open file descriptor (and might be not suitable to watch huge file hierarchies unless you can afford raising that limit so high), perform better and scale better than periodically polling a file system hierarchy. I've made tests with magnetic disks and SSDs and with file hierarchies of different sizes, going from order 0 to 10^6 and the results are consistent.

@remy, you might want to perform some measurements yourself. Based on my experience, I'm not surprised users are reporting resource drainage and low performance. Maybe you could try to use some library that decouples nodemon from the specific change notification API (such as libfswatch) and that provides a polling mechanism for platforms where no other monitor is available.

Cheers,

Enrico

simonexmachina commented 9 years ago

Well said, @emcrisostomo. :smile:

@remy, I'm not sure what you've done, but the new version of nodemon does seem to be much better :+1: What did you do?

remy commented 9 years ago

I merged a pull request that ran find the way I wanted, and correctly prunes out the ignored directories.

I'm not planning to do any work here partly because it works and the complaints about CPU have been based on 1.2.x (not the fixed 1.3.x).

I'm of course open to pull requests that implements this full with tests and 100% cross platform support (ie. no less than what nodemon supports today).

I'll close this issue in favour of a (potential) PR.

@aexmachina is already reporting nodemon 1.3 is good which hopefully supports what I've been saying.