1386040 - ./mach watch doesn't work on some files without passing --purgecaches to ./mach run

Status: RESOLVED FIXED

Description

[Brian Grinstead [:bgrins]]

I'm seeing this when using the `./mach watch` command from Bug 1384241 (OSX, artifact build)

STR:
Terminal 1: `./mach watch`
At the top of browser/components/nsBrowserGlue.js, add `dump("file changed\n");`
Terminal 2: `./mach run`
No dump statement appears

If I quit the running instance do `./mach run --purgecaches` the dump statement appears as expected

Comment 1

[Brian Grinstead [:bgrins]]

A relatively easy fix would be to always pass --purgecaches to mach run in artifact builds. But that may be papering over a problem with the command. I see there are similar issues in certain cases with `./mach build faster` (like Bug 1368699), although I'm not seeing that issue myself. If I do this then the dump statement does appear as expected:

At the top of browser/components/nsBrowserGlue.js, add `dump("file changed\n");`
`./mach build faster`
`./mach run`

Comment 2

[Brian Grinstead [:bgrins]]

When I do the STR for comment 0 (not working, mach watch)

  > find objdir.noindex -name .purgecaches
  objdir.noindex/dist/bin/browser/.purgecaches

When I do the STR for comment 1 (is working, mach build faster)

  > find objdir.noindex -name .purgecaches
  objdir.noindex/dist/bin/browser/.purgecaches
  objdir.noindex/dist/Nightly.app/Contents/Resources/browser/.purgecaches

Comment 3

[Gregory Szorc [:gps]]

We may want `mach run` to automagically purge these caches. I don't know the implications well enough to say one way or another.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Gregory Szorc [:gps] from comment #3)
> We may want `mach run` to automagically purge these caches. I don't know the
> implications well enough to say one way or another.

I also don't know the implications, but that would fix this problem and also things like Bug 1368699. Going to needinfo Mossop for feedback. It looks like there are 3 ways right now to purge caches (introduced in Bug 531886):

1) the .purgecaches file, which allows for automatic purging in local builds (the .purgecaches file is written by the build system)
2) a -purgecaches argument which causes all caches to be skipped
3) a MOZ_PURGE_CACHES environment variable which acts the same as the -purgecaches argument

IIUC the solution would be to either:
1) Track down and fix any situations where the build system isn't writing .purgecaches file properly (like here, Bug 1368699, Bug 1211957)
2) Don't rely on .purgecaches and instead always purge by default in local builds. I haven't done measurements but I guess the main reason for .purgecaches is performance (especially in debug builds).

A fairly simple workaround would be to make `./mach run` always purge caches in certain conditions (opt builds, for instance). But this would leave the broken behavior when opening a locally built binary directly without using mach.

Comment 5

[Gregory Szorc [:gps]]

glandium: do you have an opinion here? (Also, did I ask you about purgecaches recently? I swear I asked someone about it in the past month or two and it's really bothering me that I can't recall specifics.)

My naive opinion is that if these caches are for startup perf, then it seems like not using them for local builds seems like a win given the track record of "broken" builds from their use. As long as there is an escape hatch so people can test the "with caches enabled" code paths, I think we could live without them by default.

Comment 6

[Dave Townsend [:mossop] (he/him)]

Yes the caches are for performance reasons. The difference should be pretty minimal in general, stuff is cached after first load. Obviously I'd prefer if we could keep the behaviour and fix the bugs around the build system but I'm assuming that is difficult so otherwise I don't think too many folks will miss it if we cleared the cache on startup for every local build.

Comment 7

[Gregory Szorc [:gps]]

I'm not opposed to a hacky fix like "purge caches during every build system invocation." That feels less evil than purging caches for every `mach run`, as I think caches should only be purged if the build changed, not if you ran Firefox. I just don't know enough about these caches to know what is appropriate :/

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Gregory Szorc [:gps] from comment #7)
> I'm not opposed to a hacky fix like "purge caches during every build system
> invocation." That feels less evil than purging caches for every `mach run`,

Good point, having it connected to the build would be a lot better than having it connected to the run

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Gregory Szorc [:gps] from comment #7)
> > I'm not opposed to a hacky fix like "purge caches during every build system
> > invocation." That feels less evil than purging caches for every `mach run`,
> 
> Good point, having it connected to the build would be a lot better than
> having it connected to the run

It would be easy to have |mach watch| touch .purgecaches.  I'm more interested in why |mach build faster| doesn't.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Gregory Szorc [:gps] from comment #7)
> I'm not opposed to a hacky fix like "purge caches during every build system
> invocation." That feels less evil than purging caches for every `mach run`,
> as I think caches should only be purged if the build changed, not if you ran
> Firefox. I just don't know enough about these caches to know what is
> appropriate :/

How hard would it be to purge caches on every build system invocation? If we did that perhaps we could also close out Bug 1368699. I assumed that PURGECACHES_FILES was intending to do this already (https://dxr.mozilla.org/mozilla-central/source/config/rules.mk#1592-1599).

Comment 11

[Gregory Szorc [:gps]]

It is technically easy to have the build system purge caches. I'm just not sure what the history behind cache purging is.

Comment 12

[Brian Grinstead [:bgrins]]

Just wanted to note that I've been using the mach watch + the restart keyboard shortcut workflow for a couple of weeks and it's been wonderful. I've been holding off on announcing the workflow more broadly until we can figure out a fix for this bug since I think it'll cause people to not trust it (even knowing about this bug I sometimes accidentally don't purge caches and wonder if watch broke).

I wonder if we can have a more narrow fix here as opposed to changing how the cache purging works in general. If we could bring `./mach watch` into parity with `./mach build faster` it would fix this bug. That is, `./mach build faster` always works except in some rare / hard to reproduce cases like bug 1368699 - can we make `./mach watch` rely on the same thing so that it works in the same cases?

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Just wanted to note that I've been using the mach watch + the restart
> keyboard shortcut workflow for a couple of weeks and it's been wonderful.
> I've been holding off on announcing the workflow more broadly until we can
> figure out a fix for this bug since I think it'll cause people to not trust
> it (even knowing about this bug I sometimes accidentally don't purge caches
> and wonder if watch broke).
> 
> I wonder if we can have a more narrow fix here as opposed to changing how
> the cache purging works in general. If we could bring `./mach watch` into
> parity with `./mach build faster` it would fix this bug. That is, `./mach
> build faster` always works except in some rare / hard to reproduce cases
> like bug 1368699 - can we make `./mach watch` rely on the same thing so that
> it works in the same cases?

It's not easy to do exactly what |mach build faster| does, since |mach build faster| is mostly implemented in Make and ties into a bunch of much larger schemes.  But I think it would be easy to make |mach watch| clone whatever |mach build faster| does with buildid and .purgecaches.

Comment 14

[Nick Alexander :nalexander [he/him]]

Created attachment 8944094 [details]
Bug 1386040 - Write .purgecaches sentinels during |mach watch|.

Right now, the "restart flow" that combines |mach watch| with the
Quick-Restart Firefox for Desktop shortcut key is frustrated by
inconsistencies writing the .purgecaches sentinels for the
application.  This commit uses recent work from
https://bugzilla.mozilla.org/show_bug.cgi?id=1368699 to write the
sentinels each time |mach watch| updates the object directory.

Review commit: https://reviewboard.mozilla.org/r/214410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214410/

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8944094 [details]
Bug 1386040 - Write .purgecaches sentinels during |mach watch|.

https://reviewboard.mozilla.org/r/214410/#review220402

I could probably grant r+ on this. But I'd like to see the final version of the dependent patches before signing off.

::: python/mozbuild/mozbuild/faster_daemon.py:295
(Diff revision 1)
> +            if backend_cls:
> +                output = None
> +                jobs = 1
> +                verbose = False
> +                status = 0
> +                backend_cls.postbuild(self.config_environment, output, jobs, verbose, status)

This will need renamed to `post_build`. I'm also not a fan of the short-lived variables. Just pass the arguments inline.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8944094 [details]
Bug 1386040 - Write .purgecaches sentinels during |mach watch|.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214410/diff/1-2/

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8944094 [details]
Bug 1386040 - Write .purgecaches sentinels during |mach watch|.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214410/diff/2-3/

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8944094 [details]
Bug 1386040 - Write .purgecaches sentinels during |mach watch|.

https://reviewboard.mozilla.org/r/214410/#review222870

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f99f5cd4e544
Write .purgecaches sentinels during |mach watch|. r=gps

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f99f5cd4e544