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

dev-mode file change watching is CPU-intensive #2135

Closed
bdefore opened this Issue May 9, 2014 · 14 comments

Comments

Projects
None yet
5 participants
@bdefore

bdefore commented May 9, 2014

I noticed a significant performance hit when I dropped ~800 image files into the public folder of a Meteor 0.8.1.1 test project. On my dev machine (a 2011 Macbook Air), running 'mrt' or 'meteor' would cause CPU pegging, regardless of having any clients open. Activity monitor reported in the range of 20-40%.

Reproduction steps:

Some further observations:

  • This impact would drop off proportionally as I removed files from public. For example, dropping down to about 400 image files dropped CPU load to roughly 12-20% usage.
  • Renaming 'public' to 'foo' did not have any impact.
  • Running 'meteor --release 0.6.5' on the same project leaves my CPU at around 5-7% usage, leading me to believe this is due to something introduced since then.

For now, this means I'll have to host my images outside of the Meteor project.

@bdefore

This comment has been minimized.

bdefore commented May 9, 2014

One more observation:

  • Having four copies of the images directory in the root, for ~3200 images total, caused CPU drag to range from 75-85% on 0.8.1.1. Remained 5-8% on 0.6.5

@bdefore bdefore changed the title from high cpu load when many files are in meteor project root to high cpu load running server with many files in meteor project root May 9, 2014

@bdefore

This comment has been minimized.

bdefore commented May 9, 2014

→ node -v
v0.10.26

@timhaines

This comment has been minimized.

Contributor

timhaines commented May 9, 2014

@bdefore

This comment has been minimized.

bdefore commented May 10, 2014

thanks @timhaines, useful context. before closing i'd like to have someone on meteor confirm it's only in dev env?

@timhaines

This comment has been minimized.

Contributor

timhaines commented May 10, 2014

@bdefore In that thread, David Glasser who works for Meteor says 'This only affects dev mode, since of course meteor run is not designed for production use.'

@glasser

This comment has been minimized.

Member

glasser commented May 14, 2014

This is a known issue. The Node built-in file watching APIs are buggy (we used to use them but then they started crashing) and we'd like to do something better. The Atom editor folks have a new file watching package but it doesn't quite work for us; if atom/node-pathwatcher#24 is addressed I'll look into switching to it.

@glasser glasser changed the title from high cpu load running server with many files in meteor project root to dev-mode file change watching is CPU-intensive May 14, 2014

@Sanjo

This comment has been minimized.

Contributor

Sanjo commented Jul 2, 2014

@glasser Do you already know https://github.com/shama/gaze? It looks very promising and is used by grunt and gulp.

@glasser

This comment has been minimized.

Member

glasser commented Jul 22, 2014

@Sanjo I hadn't seen that, thanks. I'm not going to have time to look into this before 0.9.0 but hopefully once that's out I can look into it... my laptop battery would appreciate it :)

@Sanjo

This comment has been minimized.

Contributor

Sanjo commented Aug 1, 2014

I have done a basic integration of gaze. I replaced the watcher that watches for file changes to restart the app. It reduced the idle CPU usage of my medium size Meteor app from 25.5% to 0.8% on a Intel Core 2 Duo with 2.4 GHz with Mac OS X 10.9.4.

devel: https://github.com/Sanjo/meteor/tree/gaze
0.8.2: https://github.com/Sanjo/meteor/tree/release/0.8.2-gaze
You have to install gaze with npm install gaze inside the dev_bundle/lib/node_modules folder (after it has been created).

@glasser

This comment has been minimized.

Member

glasser commented Aug 1, 2014

That looks like a good start! I would want it to only watch the files in our WatchSet data structure, though, instead of every file in the tree.

It also looks like there's no way to tell Gaze "hey, this is the expected hashes of these files, you should fire immediately if they've already changed". We've learned the hard way that there are race conditions if you don't do that; many of the watching methods fail to detect changes that are very soon after the watch started, or between the use of the file by the compiler and the beginning of the watch. (And we can't start the watch before the build, because we won't know ahead of time which files to watch: eg, we need to watch package source that isn't physically inside the app too.)

@Sanjo

This comment has been minimized.

Contributor

Sanjo commented Aug 1, 2014

I also tried to integrate gaze into the existing Watcher first, to make it behave exactly the same. But it doesn't work correctly yet. I did the other approach to save time, because I only wanted to see the impact of the switch. Here is the branch: https://github.com/Sanjo/meteor/tree/gaze-full.
The approach of the branch is to use the existing hashing mechanism of Meteor for the first check but use the efficient gaze watcher for watching the files.

@glasser

This comment has been minimized.

Member

glasser commented Aug 12, 2014

Looks like pathwatcher did drop their stderr spamming today.

@glasser glasser added this to the Tool Performance and Stability milestone Nov 4, 2014

benjamn added a commit to benjamn/meteor that referenced this issue Nov 14, 2014

Use fs.watch again for more efficient change detection.
The idea of using fs.watch instead of fs.watchFile has been appealing ever
since we removed the original (buggy) implementation, because file system
events are a much more efficient way to detect changes to the file system,
compared with the polling strategy used by fs.watchFile.

There are several major drawbacks to fs.watch, however.

First, the fs.watch API is inconsistent across platforms. On different
systems, different sequences of events can be emitted in rapid succession,
depending on how the file system decides to represent changes. On OS X the
name of the changed file is not even passed to the callback.

Second, the number of files that can be watched simultaneously, while
relatively high on modern machines, is dangerously within the range of a
project like Meteor.

Third, fs.watch cannot be used to watch for changes to files or
directories that do not yet exist. By contrast, fs.watchFile has no
trouble polling for non-existent files.

This commit solves these problems by

1. watching only directories, using directory change events as a cue to
   trigger file change events, which has the benefit of reducing the total
   number of simultaneous watches, since there are far fewer directories
   than the total number of files,

2. coalescing directory change events so that only the last event in a
   rapid succession of events actually triggers inspection of the
   directory's contents, and

3. falling back to watching the parent directory if a given file or
   directory does not exist, so that once the file and the intermediate
   directories are created, the corresponding change events will be fired,
   achieving semantics similar to those of fs.watchFile without polling.

While I have run this code and verified that it eliminates high CPU usage
and handles file changes correctly, it would definitely be great to add
some tests to prevent regressions.

Fixes meteor#2135.

benjamn added a commit to benjamn/meteor that referenced this issue Nov 14, 2014

Use fs.watch again for more efficient change detection.
Summary:
The idea of using `fs.watch` instead of `fs.watchFile` has been appealing
ever since we removed the original (buggy) implementation, because file
system events are a much more efficient way to detect changes to the file
system, compared with the polling strategy used by `fs.watchFile`.

There are several major drawbacks to `fs.watch`, however.

First, the `fs.watch` API is inconsistent across platforms. On different
systems, different sequences of events can be emitted in rapid succession,
depending on how the file system decides to represent changes. On OS X the
name of the changed file is not even passed to the callback.

Second, the number of files that can be watched simultaneously, while
relatively high on modern machines, is dangerously within the range of a
project like Meteor.

Third, `fs.watch` cannot be used to watch for changes to files or
directories that do not yet exist. By contrast, `fs.watchFile` has no
trouble polling for non-existent files.

This commit solves these problems by

1. watching only directories, using directory change events as a cue to
   trigger file change events, which has the benefit of reducing the total
   number of simultaneous watches, since there are far fewer directories
   than the total number of files,

2. coalescing directory change events so that only the last event in a
   rapid succession of events actually triggers inspection of the
   directory's contents, and

3. falling back to watching the parent directory if a given file or
   directory does not exist, so that once the file and the intermediate
   directories are created, the corresponding change events will be fired,
   achieving semantics similar to those of `fs.watchFile` without polling.

While I have run this code and verified that it eliminates high CPU usage
and handles file changes correctly, it would definitely be great to add
some tests to prevent regressions.

Fixes meteor#2135.

Test Plan: `meteor run` and try adding/changing/removing files/directories

Reviewers: glasser

CC: nim

Differential Revision: https://phabricator.meteor.com/D922

benjamn added a commit to benjamn/meteor that referenced this issue Nov 14, 2014

Use fs.watch again for more efficient change detection.
Summary:
The idea of using `fs.watch` instead of `fs.watchFile` has been appealing
ever since we removed the original (buggy) implementation, because file
system events are a much more efficient way to detect changes to the file
system, compared with the polling strategy used by `fs.watchFile`.

There are several major drawbacks to `fs.watch`, however.

First, the `fs.watch` API is inconsistent across platforms. On different
systems, different sequences of events can be emitted in rapid succession,
depending on how the file system decides to represent changes. On OS X the
name of the changed file is not even passed to the callback.

Second, the number of files that can be watched simultaneously, while
relatively high on modern machines, is dangerously within the range of a
project like Meteor.

Third, `fs.watch` cannot be used to watch for changes to files or
directories that do not yet exist. By contrast, `fs.watchFile` has no
trouble polling for non-existent files.

This commit solves these problems by

1. watching only directories, using directory change events as a cue to
   trigger file change events, which has the benefit of reducing the total
   number of simultaneous watches, since there are far fewer directories
   than the total number of files,

2. coalescing directory change events so that only the last event in a
   rapid succession of events actually triggers inspection of the
   directory's contents, and

3. falling back to watching the parent directory if a given file or
   directory does not exist, so that once the file and the intermediate
   directories are created, the corresponding change events will be fired,
   achieving semantics similar to those of `fs.watchFile` without polling.

While I have run this code and verified that it eliminates high CPU usage
and handles file changes correctly, it would definitely be great to add
some tests to prevent regressions.

Fixes meteor#2135.

Test Plan: `meteor run` and try adding/changing/removing files/directories

Reviewers: glasser

CC: nim

Differential Revision: https://phabricator.meteor.com/D922
@Sanjo

This comment has been minimized.

Contributor

Sanjo commented Nov 16, 2014

Awesome. Thanks for fixing this. :-)

benjamn added a commit to benjamn/meteor that referenced this issue Nov 16, 2014

Use fs.watch again for more efficient change detection.
Summary:
The idea of using `fs.watch` instead of `fs.watchFile` has been appealing
ever since we removed the original (buggy) implementation, because file
system events are a much more efficient way to detect changes to the file
system, compared with the polling strategy used by `fs.watchFile`.

There are several major drawbacks to `fs.watch`, however.

First, the `fs.watch` API is inconsistent across platforms. On different
systems, different sequences of events can be emitted in rapid succession,
depending on how the file system decides to represent changes. On OS X the
name of the changed file is not even passed to the callback.

Second, the number of files that can be watched simultaneously, while
relatively high on modern machines, is dangerously within the range of a
project like Meteor.

Third, `fs.watch` cannot be used to watch for changes to files or
directories that do not yet exist. By contrast, `fs.watchFile` has no
trouble polling for non-existent files.

This commit solves these problems by

1. watching only directories, using directory change events as a cue to
   trigger file change events, which has the benefit of reducing the total
   number of simultaneous watches, since there are far fewer directories
   than the total number of files,

2. coalescing directory change events so that only the last event in a
   rapid succession of events actually triggers inspection of the
   directory's contents, and

3. falling back to watching the parent directory if a given file or
   directory does not exist, so that once the file and the intermediate
   directories are created, the corresponding change events will be fired,
   achieving semantics similar to those of `fs.watchFile` without polling.

While I have run this code and verified that it eliminates high CPU usage
and handles file changes correctly, it would definitely be great to add
some tests to prevent regressions.

Fixes meteor#2135.

Test Plan: `meteor run` and try adding/changing/removing files/directories

Reviewers: glasser

CC: nim

Differential Revision: https://phabricator.meteor.com/D922

benjamn added a commit to benjamn/meteor that referenced this issue Nov 18, 2014

Use fs.watch again for more efficient change detection.
Summary:
The idea of using `fs.watch` instead of `fs.watchFile` has been appealing
ever since we removed the original (buggy) implementation, because file
system events are a much more efficient way to detect changes to the file
system, compared with the polling strategy used by `fs.watchFile`.

There are several major drawbacks to `fs.watch`, however.

First, the `fs.watch` API is inconsistent across platforms. On different
systems, different sequences of events can be emitted in rapid succession,
depending on how the file system decides to represent changes. On OS X the
name of the changed file is not even passed to the callback.

Second, the number of files that can be watched simultaneously, while
relatively high on modern machines, is dangerously within the range of a
project like Meteor.

Third, `fs.watch` cannot be used to watch for changes to files or
directories that do not yet exist. By contrast, `fs.watchFile` has no
trouble polling for non-existent files.

This commit solves these problems by

1. watching only directories, using directory change events as a cue to
   trigger file change events, which has the benefit of reducing the total
   number of simultaneous watches, since there are far fewer directories
   than the total number of files,

2. coalescing directory change events so that only the last event in a
   rapid succession of events actually triggers inspection of the
   directory's contents, and

3. falling back to watching the parent directory if a given file or
   directory does not exist, so that once the file and the intermediate
   directories are created, the corresponding change events will be fired,
   achieving semantics similar to those of `fs.watchFile` without polling.

While I have run this code and verified that it eliminates high CPU usage
and handles file changes correctly, it would definitely be great to add
some tests to prevent regressions.

Fixes meteor#2135.

Test Plan: `meteor run` and try adding/changing/removing files/directories

Reviewers: glasser

CC: nim

Differential Revision: https://phabricator.meteor.com/D922

@benjamn benjamn closed this in a3a1739 Nov 26, 2014

@benjamn

This comment has been minimized.

Member

benjamn commented Nov 26, 2014

This activity log has taught me not to mention an issue in a commit message until I'm ready to push it. Sorry for the noise!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment