Refactor Watcher to not poll? about broccoli HOT 34 CLOSED

Nah. LiveReloading is king, so constant watching is mandatory.

If you'd like you could develop a great cross platform file watching lib, though :) Polling is certainly not the best we can do. But it definitly works and is reliable.

from broccoli.

Okay now I had a closer look. So what you like to do is hash first and only build if it is necessary.

Just an idea: Introduce builder.buildIfNecessary() and watcher only polls this function every 100ms

Putting it in the watcher seems strange, though, because a watcher should actually watch :)

from broccoli.

I would like to figure out the correct jobs to be done by the Builder, Watcher, middleware, and server.

It seems like the middleware should simply take a Builder instance and do builder.buildIfNeeded(), as you suggest, for every request that comes in the middleware, then serve the built file.

The Watcher should be orthogonal to the middleware and also take a Builder instance, but its job is to poll the filesystem for changes and command the Builder instance to buildIfNeeded() every 100ms.

This could allow the server to be used with or without constantly polling the filesystem:

Serve and build-if-needed on every request:

$ broccoli serve

Serve and watch filesystem for live-reload:

$ broccoli serve --watch

Serve and watch filesystem (poll every 500ms) for live-reload:

$ broccoli serve --watch 500

This separation of concerns would also mean that in my Express app where I'm using the Broccoli middleware directly (during development), I can choose whether or not its watching the filesystem by simply creating a Watcher instance that's bound to my Builder instance.

var tree    = broccoli.loadBrocfile(),
    builder = new broccoli.Builder(tree),
    watcher = new Watcher(builder);

app.use(middleware(builder));

watcher.on('change', function () {
    console.log('BUILD!');
});

from broccoli.

Ah jo, my proposal doesn't work in that form because it can't signify when a rebuild occured if builder.buildIfNecessary() just returns the directory like builder.build()

Edit: We submitted these two posts at the same time

from broccoli.

Ah jep that makes sense. Maybe separating the two is the logical thing to do. The only thing left to solve is that if the middleware uses buildIfNeeded then we have a slight delay that might be avoidable. Ah maybe that's neglectable.

from broccoli.

Okay I spotted something else. Above you propose that the middleware should use builder.buildIfNeeded(). That's not good because it's commonly called multiple times in succession (images, css, js, html). builder.build() doesn't work either (even worse :)

from broccoli.

Watcher.prototype.check = function() {
  this.current = this.builder.buildIfNecessary()
  this.current
    .then(function(result) {
      if (result.rebuilt) { this.emit('change', directory) }
      return result.directory
     }.bind(this))
    .catch(function(error) { this.emit('error', error) }.bind(this))
    .finally(function() {
      setTimeout(this.check.bind(this), 100)
    })
}

Maybe builder.buildIfNeeded() should resolve to { directory: '...', rebuilt: false/true }

BTW I don't think @joliss will like the --watch option. Common case is that you want rebuilding on file changes. Maybe --no-watch

from broccoli.

The only thing left to solve is that if the middleware uses buildIfNeeded then we have a slight delay that might be avoidable. Ah maybe that's neglectable.

I did a test with 1000 small ES6 modules and it stats all the files in 30ms on my rMBP (SSD). It's also likely that multiple stats will happen per HTML page that's pulling down static assets because of the interval timer hitting during before the load event fires in the browser.

BTW I don't think @joliss will like the --watch option. Common case is that you want rebuilding on file changes.

The files will still be rebuilt if they change without the --watch option, because they would be checked on each request into the server.

What I'm trying to minimize is the CPU usage and needless stat-ing that's happening all the time. I would rather turn this around such that files are only checked for changes and built when they are requested through the server/middleware.

This way Broccoli is would be doing nothing in the background, until when I refresh my browser, at which time it would stat the files and rebuilt if needed, then serve the assets. If someone wanted to use the live-realoadng feature, then --watch would come in.

@joliss let me know if you'd be interested in this, and I can propose a PR which might be the easiest way to see what I'm suggesting…

from broccoli.

I imagine those doing cross continental flights may appreciate a low-power mode. (This is based on the assumption that the stating is causing a noticeable power drain)

from broccoli.

So a single page consists of many assets (JS, CSS, PNG). Are you going to stat everything for every request? That might add up.

from broccoli.

@joliss Jep that's what I meat before. But that can be addressed the same way we do it now: 100ms interval in witch we say it's fresh enough

from broccoli.

I tested this last week with over 1000 simple ES6 modules, and stating all files took ~30ms per request. So this can add up, but on a normal sized project, it's ~1–2ms overhead.

With the current interval approach, it's likely to still incur 2 or more all-file-stats per page load since requests for some of the static assets will fall into another interval run.

from broccoli.

I would be curious for someone to give me real #'s how much this reduces my battery life. I have a hard time gauging the actual impact.

from broccoli.

• buildIfNeeded() (public)
  Rebuilds if changed files were detected.
  If a build is currently in progress, then it delays the check
  until the current build has finished, checks then and rebuilds
  if required.
  @return {Promise} Resolves to the following Object:
  • {string} directory Output directory
  • {boolean} rebuilt Is true if a rebuild occured
• build() (public)
  Starts the build process.
  Calling this method multiple times in a row triggers only one build because the
  build will only start in the next tick of the process.
  If currently a build is running, it schedules a build that runs after the current
  build has finished. Calling this method multiple times during that period won't
  schedule more than one build.
  @return {Promise} Resolves to output directory
• isRebuildRequired() (private)
  Checks whether a rebuild is required.
  Used by buildIfNeeded() internally.
  It answers with the response from the last check if it was less then 99ms ago. If a check
  is currently already in progress, then it waits until that has finished and reports its result.
  @return {Promise} Resolves to true/false if a rebuild is required/not required.

The idea is that the watcher calls buildIfNeeded() every 100ms and the middleware uses it, too.

The approach I just described serves both use cases. It works with and without active file watching. And when multiple files are accessed in quick succession, then it won't check everytime, but only after some time has elapsed.

@joliss What about merging watcher into builder? You won't ever have more than one watcher per builder. It's more like builder.autoRebuild = true

What do you think? What did I overlook?

from broccoli.

I agree the current situation is not great, as it drains battery.

@ericf 1k files is unfortunately not an unreasonable scenario, from what I've heard. Broccoli needs to be able to deal with this order of magnitude files performantly.

@MajorBreakfast This API seems pretty reasonable, if we decided to go down this path.

I'm generally a bit unhappy though with having two separate code paths (polling repeatedly, and stat'ing on HTTP request). I'm also not sure that we can cache the state for 100ms as an optimization; you can probably construct situations where this causes us to miss changes entirely.

If we can get it, my preferred solution would be something inotify-based, with polling as fallback. Essentially, someone has to write or integrate a cross-platform inotify library in a race-condition-free manner. "Race-condition-free" meaning, we must never miss changes, even if the file system is constantly changing. It seems to me that the logic goes something like this:

1. Stat tree.
2. Run build.
3. Recursively set up inotify watchers. (Treat failures as immediate change event; it means the file system is changing under our feet.)
4. Stat tree again, and compare to last stats. If anything has changed, go to (2).
5. Wait for inotify event.
6. Wait 50ms. This is not necessary for the "no changes missed" guarantee, but it helps avoid double rebuilds for file changes coming in short succession.
7. Go to (1).

Step 4 is crucial because it guarantees that we never miss changes.

I'm not sure where we stand in terms of basic inotify support, but it seems like this should be possible, and not be terribly complicated either. We might be able to have it live in a standalone library, which would be really awesome because it could replace all the half-broken inotify implementations floating around right now.

from broccoli.

I'm also not sure that we can cache the state for 100ms as an optimization; you can probably construct situations where this causes us to miss changes entirely.

True but that's currently also the case since the watcher polls in 100ms intervals. I think we're only on the save side if we use a polling free algorithmn like you propose.

from broccoli.

Maybe we can simplify the api to builder.build({forceRebuild: false }) (returns a hash) and builder.autoRebuild = true. With the force option if you want to rebuild even if no file changed (for updating a date or something)

from broccoli.

About the watching library:

• never throw
• never miss a change
• Could make use of https://github.com/audreyt/node-webworker-threads (maybe not supported on windows)

from broccoli.

https://github.com/paulmillr/chokidar is widely used and very mature.

from broccoli.

As somebody who's about to take a cross-continental flight with ember server running, I'm suddenly interested in this issue.

I just hacked up a quick custom version using chokidar and it's working nicely. I'll try and generate some real battery usage numbers to see if its worth it. (I just hard-coded paths and ignores that are relevant to me -- doing it properly would involve intelligently pulling those things out of broccoli's config.)

from broccoli.

👍

from broccoli.

I know that @joliss has looked at chokidar before. I'm not sure why she dropped it. But I think it was in the very early stages of broccoli when she hadn't yet had the idea of passing trees around.

@joliss I'm not so sure now that any hashing after setting up of watchers is necessary: Watcher libs first set up watchers on the parent folder. Even if a new file gets added after it queried for the child entries of the folder the file will still get its watcher because the addition of the file will trigger a change event on the parent folder and in that we set up watchers for any files that don't yet have one. I don't see a problem here. We should switch to an external watcher lib like chokidar instead of polling. Worst thing that can happen is that we miss a file change. Although, I think that's unlikely. It's not that broccoli could produce a corrupt build as a consequence of a missed file change because the caching doesn't rely on file change events. I think we should just go for it.

from broccoli.

It seems that Chokidar is designed to just keep running, rather than being re-set up after each build. With permanently running watchers, it's probably easy to miss changes, because there are so many opportunities for race conditions. That must never happen. But I may be wrong.

from broccoli.

Not sure I understand. Chokidar is used in brunch build and brunch watch. Handles both cases great. Just use persistent: false.

Chokidar is also the only module that utilises OS X internals for fast low-CPU non-polling fs watching.

from broccoli.

Handles both cases great. Just use persistent: false.

OK, very cool. We should give it a try.

from broccoli.

I agree. We should give it a try. I'm sure many users are quite happy with it.

Btw. the persistent option is for keeping the node process running even if there is nothing to do left. It's the same as for the server. Not for keeping any watchers around. So you two don't mean the same thing :)

from broccoli.

Obviously this wouldn't be useful for Windows or Linux but support for https://www.npmjs.org/package/fsevents would be awesome and would solve this issue on Macs at least.

from broccoli.

Chokidar actually uses fsevents on Macs (as of release 0.8)—https://github.com/paulmillr/chokidar/blob/master/CHANGELOG.md

from broccoli.

@joliss is this okay to take and work on? I don't want to go too far off the reservation but maybe if you wanted to lay down some ground rules or ground work I can donate some spare cycles to this effort.

@ef4 can I steal your pre-EmberConf code?

Impetus: I accidentally included a node_modules directory in a Broccoli tree today and started getting more than 50% CPU usage on idle with 10-12 second build times. Watching gets pathologically bad really quick. Also, I hate feeling like I need to be plugged in to work.

from broccoli.

I accidentally included a node_modules directory in a Broccoli tree today and started getting more than 50% CPU usage on idle with 10-12 second build times.

The same thing happened to me and I ended up having to create a little class that avoids calling readTree(): https://github.com/yui/pure-site/blob/master/lib/map-files.js

from broccoli.

@nathanhammond Sorry, I don't think I still have it. It was a quick and dirty experiment and I never bothered to fork broccoli and commit it anywhere. But take a look at broccoli/lib/watcher.js, it's short and sweet and pretty easy to mess with.

from broccoli.

Any progress?

from broccoli.

+1

from broccoli.

after long wait, broccoli ships with broccoli-sane-watcher by default. (WHich defaults to a non polling watcher).

from broccoli.