Closed Bug 1384241 Opened 7 years ago Closed 7 years ago

Add daemon that triggers (partial) |mach build faster| builds on filesystem input changes (using pywatchman)

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Depends on 1 open bug, Blocks 5 open bugs)

Details

Attachments

(3 files, 8 obsolete files)

59 bytes, text/x-review-board-request
gps
: review+
Details
59 bytes, text/x-review-board-request
gps
: review+
Details
15.34 KB, patch
Details | Diff | Splinter Review
This ticket tracks a subset of the functionality of Bug 1176557, namely triggering (partial) |mach build faster| builds when files change in the topsrcdir.
To test, I've been using wscat, like:

⋊> ~ wscat -c 'ws://localhost:8080/websocket'                                                                                                                                                                                       12:37:48
connected (press CTRL+C to quit)
< {"type": "change", "change": {"inputs": ["/Users/nalexander/Mozilla/gecko/browser/themes/shared/aboutSocialError.css"], "unrecognized": [], "outputs": ["browser/chrome/browser/skin/classic/browser/aboutProviderDirectory.css", "browser/chrome/browser/skin/classic/browser/aboutSocialError.css"]}, "sequence": 0}

That's the change message coming from:

⋊> ~/M/gecko touch browser/themes/shared/aboutSocialError.css
Comment on attachment 8890029 [details]
Bug 1384241 - Part 2: Add |mach watch|: pywatchman based |mach build faster| daemon.

https://reviewboard.mozilla.org/r/161082/#review166466

I'm not done with the review. But please add a new patch that adds watchman detection to moz.configure and check for the presence of watchman before attempting to use it.

Ideally, we'd pass the path to watchman to pywatchman. However, it looks like the binary name 'watchman' is hardcoded in pywatchman. I'm pretty sure I've signed Facebook's CLA, so I'll create a PR for that.

::: python/mozbuild/mozbuild/mach_commands.py:315
(Diff revision 1)
> +
> +    @Command('watch', category='build', description='Watch and re-build the tree.')
> +    def watch(self):
> +        """Watch and re-build the source tree."""
> +        self._activate_virtualenv()
> +        self.virtualenv_manager.install_pip_package('pywatchman==1.3.0')

I'm fine installing pywatchman from pip for now. However, pywatchman has a C extension. This will likely fail to install on Windows because building Python 2.7 extensions on Windows requires a copy of the Visual Studio 2008 toolchain, which most people don't have installed.

We'll want error checking around this package install. And, we may want to skip it entirely on Windows until we hook up binary distribution of a pywatchman wheel.
Comment on attachment 8890028 [details]
Bug 1384241 - Part 1: Add input to output mappings.

https://reviewboard.mozilla.org/r/161080/#review166556

I approve of the new features! The new `inputs()` method could be beneficial for writing different build backends!

But r- because a v2 of this series will be required.

I would encourage you to split this part to its own bug so it can be landed sooner, as mozpack is surprisingly active this week and you are likely to get bit rotted.

::: python/mozbuild/mozpack/copier.py:183
(Diff revision 1)
> +        for output, file in self:
> +            output = mozpath.normpath(output)
> +            for input in file.inputs():
> +                input = mozpath.normpath(input)
> +                tree[input].add(output)
> +        return dict(tree.iteritems())

Nit: pretty sure you can exclude `.iteritems()` here since `dict(<dict>)` does a shallow copy.

::: python/mozbuild/mozpack/files.py:487
(Diff revision 1)
>          if not dest.exists():
>              errors.fatal("Required existing file doesn't exist: %s" %
>                  dest.path)
>  
> +    def inputs(self):
> +        return iter(())

Meh.

Most consumers likely don't care that this method returns an iterator. I would make the API contract "returns an iterable" and return lists or sets instead.

::: python/mozbuild/mozpack/files.py:506
(Diff revision 1)
> +        pp = Preprocessor(defines=self.defines, marker=self.marker)
> +        pp.setSilenceDirectiveWarnings(self.silence_missing_directive_warnings)

I'm slightly concerned about the duplication of code around invoking the preprocessor here and in `copy()`. It would be very easy for these to get out of sync and for bugs to form.

I think it's fine since the code is minimal and we don't touch it often. But if you wanted to refactor the "invoke the preprocessor" code to a standalone function, I'd approve!

::: python/mozbuild/mozpack/files.py:510
(Diff revision 1)
> +    def inputs(self):
> +        pp = Preprocessor(defines=self.defines, marker=self.marker)
> +        pp.setSilenceDirectiveWarnings(self.silence_missing_directive_warnings)
> +
> +        with open(self.path, 'rU') as input:
> +            with open(os.devnull, 'w') as output:

I'm honestly not sure if this is better or worse than writing to a throwaway `io.BytesIO` or a no-op object. It probably doesn't matter.
Attachment #8890028 - Flags: review?(gps) → review-
Comment on attachment 8890029 [details]
Bug 1384241 - Part 2: Add |mach watch|: pywatchman based |mach build faster| daemon.

https://reviewboard.mozilla.org/r/161082/#review166560

First, thank you for writing this patch. This feature is obviously a good idea and great things will eventually come from it.

There are many review comments. I view this feature much like artifact builds were in the beginning: experimental. "Perfect is the enemy of good" definitely applies and we shouldn't get bogged down in minutia. I'm all for landing *something* and then refactoring like crazy as necessary.

I've left a few higher-level/architecture comments inline.

Another idea I'd like you to consider is refactoring this as a build backend. That way, the input/output map can be regenerated as part of running `config.status` rather than the somewhat hacky approach of parsing the fastermake's `.track` files. Or at the very least, it will allow us to share backend-related code easier. As it stands, code in this new file exists outside the build system core and it is essentially bolted on top. It feels fragile and prone to breakage.

If this were integrated as a build backend, I imagine we'd have to use a true daemon. We could start the daemon automatically during the first build and it would be running in the background. Another benefit to integrating it into the actual build system is you'll have better control over locks and concurrent execution. I imagine we'll get into some states where an existing, user-initiated build is triggering topsrcdir changes (e.g. .pyc file updates). We'll probably want to defer subscription updates until other build processes have ended. Watchman has a facility for this. But obviously you need signaling in place to tell it when to pause subscriptions. That's where having a formal build backend could be useful.

TBH, I'm not sure where to draw the line in terms of acceptable quality for an initial landing. Perfect is the enemy of good. But I don't want to incur too much technical debt either. This will be a like artifact builds and will introduce a lot of "traffic" when it lands. If we can minimize the number of bugs by choosing a better architecture up front, I think that's worth sacrificing some speed for.

I definitely want to think a bit more about the future "architecture" for a background daemon and file watching. I also want to error on the side of being able to achieve fast builds with as little user involvement as possible. For example, artifact builds are amazing but since they require a manual opt-in, there are developers not seeing their benefits. I would very much like to devise a filesystem watching/building feature to require as little user involvement as possible. It's fine if we don't have that initially. But we should have a transition plan.

Again, this will be a killer feature. If you want help hacking on it, I think I can find time. But you know me - I'm always over-committed, so no promises :)

::: commit-message-ef2c4:5
(Diff revision 1)
> +- watching in a running Python instance, rather than invoking a
> +  process.  I want to avoid Python startup overhead, which dwarfs the
> +  build faster incremental updates.

Huh? `mach`'s startup overhead should be under 100ms on a new machine. And there's a lot of room to optimize that if it becomes a problem. Startup overhead of a non-mach process will be dozens of milliseconds shorter.

I think it is worth discussing this architecture in more detail because daemons are hard. Although I see from your next patch that you have a very compelling use of a daemon. And there's other things I've wanted to shoehorn into a generic build/mach daemon over the years. While a generic daemon is scope bloat for this bug, it does skew my (eventual) architecture preference towards a daemon.

::: commit-message-ef2c4:17
(Diff revision 1)
> +- watching subdirectories of topsrcdir.  I particularly want to avoid
> +  topobjdir-in-topsrcdir.

Assuming Mercurial is being used, it will have a watch of the entire topsrcdir. So I don't think this optimization is worth while: just watch the entire topsrcdir.

If the overhead of watchman watching a topobjdir-in-topsrcdir is significant (it is multiple CPU seconds during a build but I haven't measured exactly), we can warn/ban topobjdir-in-topsrcdir in configure when watchman is being used.

Finally, another benefit of subscribing to the root directory is that watchman is VCS aware and it will avoid emitting events while a Mercurial or Git transaction is ongoing. i.e. it waits for the working directory operation to "settle" before informing consumers. You definitely want this.

::: python/mozbuild/mozbuild/faster_daemon.py:69
(Diff revision 1)
> +        defines = dict((name, self.config_environment.defines[name])
> +                       for name in self.config_environment.defines
> +                       if name not in self.config_environment.non_global_defines)

Ewww. Could I get you to add a helper property to configenvironment.py instead? Or ignore if you don't want the scope bloat :)

::: python/mozbuild/mozbuild/faster_daemon.py:72
(Diff revision 1)
> +        defines.update({
> +            'AB_CD': 'en-US',
> +            'BUILD_FASTER': 1,
> +        })
> +        defines.update({
> +            'BOOKMARKS_INCLUDE_DIR': mozpath.join(self.config_environment.topsrcdir,

Please add inline comment why all these are necessary.

::: python/mozbuild/mozbuild/faster_daemon.py:74
(Diff revision 1)
> +        defines = dict((name, self.config_environment.defines[name])
> +                       for name in self.config_environment.defines
> +                       if name not in self.config_environment.non_global_defines)
> +        defines.update({
> +            'AB_CD': 'en-US',
> +            'BUILD_FASTER': 1,

Values should be strings.

::: python/mozbuild/mozbuild/faster_daemon.py:82
(Diff revision 1)
> +    @property
> +    def file_copier(self):
> +        if self._file_copier is None:

You can use `@mozbuild.util.memoized_property` for this cached property pattern.

::: python/mozbuild/mozbuild/faster_daemon.py:87
(Diff revision 1)
> +            finder = FileFinder(mozpath.join(self.config_environment.topobjdir, 'faster'))
> +            for path, f in finder.find('*.track'):
> +                manifest = InstallManifest(fileobj=f.open())

This feels like a layering violation. I assume similar code exists somewhere in a build backend? Is there room to consolidate?

::: python/mozbuild/mozbuild/faster_daemon.py:133
(Diff revision 1)
> +
> +        return watches
> +
> +    def subscribe_to_dir(self, name, dir_to_watch):
> +        query = {
> +            'expression': ['true'],

When you change this to watch the entire topsrcdir, be sure to exclude .hg and .git. e.g. 

```
'not', ['anyof', ['dirname', '.hg'],
                 ['name', '.hg', 'wholename']
       ]
```

(That's just for Mercurial, obviously.)

::: python/mozbuild/mozbuild/faster_daemon.py:145
(Diff revision 1)
> +        root = watch['watch']
> +        if 'relative_path' in watch:
> +            query['relative_root'] = watch['relative_path']
> +
> +        # Get the initial clock value so that we only get updates.
> +        query['since'] = self.client.query('clock', root)['clock']

I think this may want `sync_timeout` and/or `empty_on_fresh_instance` defined.

::: python/mozbuild/mozbuild/faster_daemon.py:166
(Diff revision 1)
> +
> +            for dat in data:
> +                files |= set([mozpath.join(self.config_environment.topsrcdir, name, f)
> +                              for f in dat.get('files', [])])
> +
> +            files = set([f for f in files if os.path.isfile(f)])

Watchman 3.1+ supports a "type" field in the subscription. If you receive that, you can avoid the stat() here.

Even better, the query expression can limit to certain types so this filtering is done in watchman itself!

::: python/mozbuild/mozbuild/faster_daemon.py:172
(Diff revision 1)
> +
> +        return files
> +
> +    def incremental_copy(self, copier, force=False, verbose=True):
> +        # Just like the 'repackage' target in browser/app/Makefile.in.
> +        if 'cocoa' == self.config_environment.substs['MOZ_WIDGET_TOOLKIT']:

I hate that the build system does this. But what are you gonna do?

::: python/mozbuild/mozbuild/faster_daemon.py:200
(Diff revision 1)
> +        '''
> +        Return an iterator of `FasterBuildChange` instances as inputs
> +        to the faster build system change.
> +        '''
> +
> +        self.client = pywatchman.client()

By default, the watchman client uses JSON. JSON is slow.

There's an `useImmutableBser` argument that tells the client to use Python objects that are immutable. This can translate to significant performance wins for large data exchanges. It is probably best to use that.

::: python/mozbuild/mozbuild/faster_daemon.py:200
(Diff revision 1)
> +        '''
> +        Return an iterator of `FasterBuildChange` instances as inputs
> +        to the faster build system change.
> +        '''
> +
> +        self.client = pywatchman.client()

You'll want a `try..finally` here so you can `self.client.close()` to ensure the watchman connection is cleaned up on function exit.

::: python/mozbuild/mozbuild/faster_daemon.py:217
(Diff revision 1)
> +                _watch_result = self.client.receive()
> +
> +                changed = self.changed_files()

It looks like the returned object is always populated. But `self.changed_files()` does a blind iteration of all subs and tests each one. This feels a bit wasteful.

Assuming you switch to the immutable bser codec, test for `hasattr(_watch_result, 'subscription')` and no-op.

::: python/mozbuild/mozbuild/faster_daemon.py:226
(Diff revision 1)
> +                    continue
> +
> +                result = FasterBuildChange()
> +
> +                for change in changed:
> +                    change = mozpath.normpath(change)

I think path normalization should be done in `changed_files()`.

::: python/mozbuild/mozbuild/faster_daemon.py:234
(Diff revision 1)
> +                        if output not in result.output_to_inputs:
> +                            result.output_to_inputs[output] = set()
> +                        result.output_to_inputs[output].add(input)

Use a `collections.defaultdict`.

::: python/mozbuild/mozbuild/mach_commands.py:318
(Diff revision 1)
> +        daemon = Daemon(self.config_environment)
> +        return daemon.watch()

OK - so this isn't exactly a "daemon" because it never detaches. Instead, the process just sits around, waiting for events or termination.

That's fine. But it does mean that users have to opt in to this and start a foreground process. That's a bit annoying.

I'd be tempted to create `mach daemon start` and `mach daemon stop` to control a background daemon. It can kill itself after N seconds of inactivity so it doesn't "leak." We could also have it self-terminate when the objdir is deleted.

But I suppose this would be a follow-up.
Attachment #8890029 - Flags: review?(gps) → review-
Comment on attachment 8890030 [details]
Bug 1384241 - Part 2: Push changes to WebSocket listeners.

https://reviewboard.mozilla.org/r/161084/#review166582

I don't know gevent, so this code is over my head. Fortunately, I know a few Mozillians who can help out when the time comes. I'd start with Tarek Ziade and Ryan Kelly.

I think the concept of starting a WebSocket server so Firefox can receive notifications about updated files is pretty rad. Opens up a lot of awesome potential workflows and features for hacking on Firefox within Firefox (which has always been a concept I've wanted to explore).

The websocket code should live outside a mach_commands.py file. We don't want to pollute mach_commands.py files. As we move to a model where we have daemons, I think we'll find that we regret a lot of the code being in mach_commands.py and not consumable as a library by non-mach processes/daemons.

It also feels like we're missing a generic primitive for subscribing Python callbacks to filesystem changes. We have 2 consumers in 2 patches. I imagine there will be more. How do we dispatch events to multiple consumers? (YAGNI may apply here.)

::: python/mozbuild/mozbuild/mach_commands.py:319
(Diff revision 1)
> +    def watch(self, verbose=False):
>          """Watch and re-build the source tree."""
>          self._activate_virtualenv()
>          self.virtualenv_manager.install_pip_package('pywatchman==1.3.0')
> +        self.virtualenv_manager.install_pip_package('bottle==0.12.13')
> +        self.virtualenv_manager.install_pip_package('gevent-websocket==0.10.1')

gevent requires compiled extensions and may have problems installing. However, they publish binary wheels, so we may get lucky.

::: python/mozbuild/mozbuild/mach_commands.py:350
(Diff revision 1)
> +            except pywatchman.CommandError as ex:
> +                print('watchman:', ex.msg, file=sys.stderr)
> +                sys.exit(1)
> +
> +            except pywatchman.SocketTimeout as ex:
> +                print('watchman:', str(ex), file=sys.stderr)
> +                sys.exit(2)
> +
> +            except KeyboardInterrupt:
> +                # Suppress ugly stack trace when user hits Ctrl-C.
> +                sys.exit(3)

It would be really nice if the watchman details were abstracted away.
Attachment #8890030 - Flags: review?(gps) → review-
Depends on: 1384396
Comment on attachment 8890028 [details]
Bug 1384241 - Part 1: Add input to output mappings.

https://reviewboard.mozilla.org/r/161080/#review166590

::: python/mozbuild/mozpack/copier.py:183
(Diff revision 1)
> +        for output, file in self:
> +            output = mozpath.normpath(output)
> +            for input in file.inputs():
> +                input = mozpath.normpath(input)
> +                tree[input].add(output)
> +        return dict(tree.iteritems())

This is really converting from a `defaultdict` to a `dict`, but your suggestion seems correct.  I'll do this.

::: python/mozbuild/mozpack/files.py:487
(Diff revision 1)
>          if not dest.exists():
>              errors.fatal("Required existing file doesn't exist: %s" %
>                  dest.path)
>  
> +    def inputs(self):
> +        return iter(())

I agree, but I always avoid returning modifiable collections to consumers, and I don't want consumers to expect ordering.  I'll consider dropping the iterator.

::: python/mozbuild/mozpack/files.py:506
(Diff revision 1)
> +        pp = Preprocessor(defines=self.defines, marker=self.marker)
> +        pp.setSilenceDirectiveWarnings(self.silence_missing_directive_warnings)

The structure of the code isn't really amenable to extracting a helper -- the dep-writing code is entangled with the consuming `copy()`.  I'm going to leave this for now.

::: python/mozbuild/mozpack/files.py:510
(Diff revision 1)
> +    def inputs(self):
> +        pp = Preprocessor(defines=self.defines, marker=self.marker)
> +        pp.setSilenceDirectiveWarnings(self.silence_missing_directive_warnings)
> +
> +        with open(self.path, 'rU') as input:
> +            with open(os.devnull, 'w') as output:

It wasn't clear to me if there was a no-op object already defined; a preliminary search didn't reveal one.  Hence, `os.devnull`... which is basically that, provided by the OS.
Depends on: 1384400
Comment on attachment 8890029 [details]
Bug 1384241 - Part 2: Add |mach watch|: pywatchman based |mach build faster| daemon.

https://reviewboard.mozilla.org/r/161082/#review167080

Thanks for the great review!  I'm excited to see you're excited!

::: commit-message-ef2c4:17
(Diff revision 1)
> +- watching subdirectories of topsrcdir.  I particularly want to avoid
> +  topobjdir-in-topsrcdir.

This VCS-aware debouncing is compelling, so I've switched to a single watch of topsrcdir.

::: python/mozbuild/mozbuild/faster_daemon.py:69
(Diff revision 1)
> +        defines = dict((name, self.config_environment.defines[name])
> +                       for name in self.config_environment.defines
> +                       if name not in self.config_environment.non_global_defines)

Done as a Pre: patch.

::: python/mozbuild/mozbuild/faster_daemon.py:72
(Diff revision 1)
> +        defines.update({
> +            'AB_CD': 'en-US',
> +            'BUILD_FASTER': 1,
> +        })
> +        defines.update({
> +            'BOOKMARKS_INCLUDE_DIR': mozpath.join(self.config_environment.topsrcdir,

I have done so, referencing the ranges I culted this from.  It might be worth pushing these hacks up into the `acdefines()` helper I declared... but it wouldn't get them out of Faster Make's `rules.mk` :(

::: python/mozbuild/mozbuild/faster_daemon.py:74
(Diff revision 1)
> +        defines = dict((name, self.config_environment.defines[name])
> +                       for name in self.config_environment.defines
> +                       if name not in self.config_environment.non_global_defines)
> +        defines.update({
> +            'AB_CD': 'en-US',
> +            'BUILD_FASTER': 1,

Fixed.

::: python/mozbuild/mozbuild/faster_daemon.py:87
(Diff revision 1)
> +            finder = FileFinder(mozpath.join(self.config_environment.topobjdir, 'faster'))
> +            for path, f in finder.find('*.track'):
> +                manifest = InstallManifest(fileobj=f.open())

You can see how I've interpreted this in the patch series I'm about to push.

::: python/mozbuild/mozbuild/faster_daemon.py:133
(Diff revision 1)
> +
> +        return watches
> +
> +    def subscribe_to_dir(self, name, dir_to_watch):
> +        query = {
> +            'expression': ['true'],

Done.

::: python/mozbuild/mozbuild/faster_daemon.py:145
(Diff revision 1)
> +        root = watch['watch']
> +        if 'relative_path' in watch:
> +            query['relative_root'] = watch['relative_path']
> +
> +        # Get the initial clock value so that we only get updates.
> +        query['since'] = self.client.query('clock', root)['clock']

Reading the documentation for `sync_timeout` at https://facebook.github.io/watchman/docs/cmd/query.html, I don't know what you intend.  I don't think you'd want `sync_timeout` to be _larger_ than the default, which is 60s.  Do you think `sync_timeout` should be 0, and we should accept inconsistent filesystem states?  In both cases I don't think this approach is going to produce a good developer experience, but it might be more useful to at least push _some_ output changes.

I think I agree about `empty_on_fresh_instance`, so I'll add that now.

::: python/mozbuild/mozbuild/faster_daemon.py:166
(Diff revision 1)
> +
> +            for dat in data:
> +                files |= set([mozpath.join(self.config_environment.topsrcdir, name, f)
> +                              for f in dat.get('files', [])])
> +
> +            files = set([