976216 - Include -purgecaches command line to Gecko for developer builds of Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

The Gecko startup cache makes it pretty difficult to develop JS modules.  |mach build mobile/android/modules && mach package && mach install| doesn't appear to clear the startup cache, meaning that changed code is never loaded.

For developer builds, when folks might be touching JS modules, we should include "-purgecaches" (or set pref nglayout.debug.disable_xul_cache = true).  This is trivial: just add the option at or around http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#327.

Comment 1

[Richard Newman [:rnewman]]

Workaround:

adb shell am force-stop org.mozilla.fennec_rnewman
adb shell run-as org.mozilla.fennec_rnewman rm -r /data/data/org.mozilla.fennec_rnewman/files/mozilla/g11xq8bf.default/startupCache

Comment 2

[Nick Alexander :nalexander [he/him]]

I just ran into this (yet again!) while adding another about: page.  This time, it only took me 3 failing build-runs to recall the solution, which I'm counting as progress.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Add AppConstants.MOZILLA_OFFICIAL. r=mfinkle

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Purge startup caches when starting Gecko in developer builds. r=mfinkle

This change only impacts developer builds.  A developer build is one
where MOZILLA_OFFICIAL is not set.

The startup cache is invalidated when the buildid changes; see [1] for
details.  For MOZILLA_OFFICIAL builds, the buildid is always bumped, so
the startup cache is always fresh on a package re-deploy to device.

Most developers re-deploy using |mach build mobile/android && mach
package && mach install| or similar.  This does not bump the buildid.
The re-deployed package will read the out-dated startup cache, leading
to frustrating inconsistencies when developing Javascript, especially
chrome content and module JSMs.

This change purges the startup caches every time Gecko is started in
developer builds.  This keeps the running Javascript consistent (which
is good for development), but incurs a startup performance
penalty (since the cache must be purged, and since the cached files must
be recompiled, etc).

[1] http://hg.mozilla.org/mozilla-central/file/901d300bb441/toolkit/xre/nsAppRunner.cpp#l2350

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 8412786 [details] [diff] [review]
Part 1: Add AppConstants.MOZILLA_OFFICIAL. r=mfinkle

Review of attachment 8412786 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/moz.build
@@ +508,5 @@
>  for var in ('MOZ_ANDROID_ANR_REPORTER', 'MOZ_LINKER_EXTRACT'):
>      if CONFIG[var]:
>          DEFINES[var] = 1
>  
> +for var in ('MOZ_UPDATER', 'MOZ_PKG_SPECIAL', 'MOZILLA_OFFICIAL'):

Why this block, and not the = 1 block above?

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #5)
> Comment on attachment 8412786 [details] [diff] [review]
> Part 1: Add AppConstants.MOZILLA_OFFICIAL. r=mfinkle
> 
> Review of attachment 8412786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/moz.build
> @@ +508,5 @@
> >  for var in ('MOZ_ANDROID_ANR_REPORTER', 'MOZ_LINKER_EXTRACT'):
> >      if CONFIG[var]:
> >          DEFINES[var] = 1
> >  
> > +for var in ('MOZ_UPDATER', 'MOZ_PKG_SPECIAL', 'MOZILLA_OFFICIAL'):
> 
> Why this block, and not the = 1 block above?

No reason; I think they are equivalent.  Agree that the earlier block is clearer; will update before landing.

Comment 7

[Richard Newman [:rnewman]]

Comment on attachment 8412786 [details] [diff] [review]
Part 1: Add AppConstants.MOZILLA_OFFICIAL. r=mfinkle

Review of attachment 8412786 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that, then. (Stealing your Saturday reviews, mfinkle!)

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8412787 [details] [diff] [review]
Part 2: Purge startup caches when starting Gecko in developer builds. r=mfinkle

Review of attachment 8412787 [details] [diff] [review]:
-----------------------------------------------------------------

I like the idea of fixing this pain point, but I have a number of questions that I think are worth answering, if only for the record.

1. Are you 100% convinced that MOZILLA_OFFICIAL is the right flag to use here? Rather than, say, MOZ_UPDATE_CHANNEL.equals("default"), or a new flag?

2. Why did you elect to make this a client-side startup operation, rather than a build step (e.g., purging caches via `adb shell` in `mach install`), or a client-side operation tied to the package upgrade intent that Android sends on first run after install?

3. Is another solution simply to always bump the build ID for every build? If not, why not?

My mild concern is that not every place we do things like benchmarking will be using builds with MOZ_OFFICIAL set (and not everyone reads log output to see your warning message), and this is really an operation that should occur after install, not on every startup.

::: mobile/android/base/GeckoAppShell.java
@@ +352,5 @@
> +        // the buildid mechanism, but most un-official builds don't bump the
> +        // buildid, so we purge here instead.
> +        if (!AppConstants.MOZILLA_OFFICIAL) {
> +            Log.w(LOGTAG, "STARTUP PERFORMANCE WARNING: un-official build: purging the " +
> +                          "startup (Javascript) caches.");

Nit: JavaScript.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #8)
> Comment on attachment 8412787 [details] [diff] [review]
> Part 2: Purge startup caches when starting Gecko in developer builds.
> r=mfinkle
> 
> Review of attachment 8412787 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the idea of fixing this pain point, but I have a number of questions
> that I think are worth answering, if only for the record.
> 
> 1. Are you 100% convinced that MOZILLA_OFFICIAL is the right flag to use
> here? Rather than, say, MOZ_UPDATE_CHANNEL.equals("default"), or a new flag?

No!  This is based on the discussions I have had with blassey in the Wednesday Mobile meeting.  I considered adding a config variable, etc, etc -- but it needs to default to on for devs, off for tbpl, etc, and it's irritating to add config variables for stuff that should be automatic.  I'd be happy to bump the logic to something like:

PURGECACHES_ON_GECKO_STARTUP = !AppConstants.MOZ_OFFICIAL && MOZ_UPDATE_CHANNEL.equals("default").

> 2. Why did you elect to make this a client-side startup operation, rather
> than a build step (e.g., purging caches via `adb shell` in `mach install`),
> or a client-side operation tied to the package upgrade intent that Android
> sends on first run after install?

My personal motivating use case is launching from Eclipse, so |mach install| did not occur to me.  It's not a bad solution, modulo that determining the profile from |adb shell| is onerous.

I considered the upgrade intent handler, but Fennec doesn't have one defined for me to throw this into!  And adding one seems... overkill.  I could be convinced otherwise, for certain.

> 3. Is another solution simply to always bump the build ID for every build?
> If not, why not?

Yes, but this does not fit the flow that is already in place (|mach build mobile/android && ...|) and the buildid is compiled into the code in places (that I don't understand) and requires relinking some libraries.  I don't want to incur the penalty.

We do, however, have an alternate mechanism around ANDROID_VERSION_CODE that gets bumped for just this purpose; see http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Makefile.in#20.

We could persist the last ANDROID_VERSION_CODE to disk or SharedPrefs and purge based on that.  That would mitigate but not remove the startup perf concern.

> My mild concern is that not every place we do things like benchmarking will
> be using builds with MOZ_OFFICIAL set (and not everyone reads log output to
> see your warning message), and this is really an operation that should occur
> after install, not on every startup.

Agreed, but it's my strong opinion loosely held that running builds not produced by TBPL for performance testing is a Bad Idea.  Toolchains and configuration flags actually matter.

Comment 10

[Nick Alexander :nalexander [he/him]]

> I considered the upgrade intent handler, but Fennec doesn't have one defined
> for me to throw this into!  And adding one seems... overkill.  I could be
> convinced otherwise, for certain.

I should add that the "purgecaches" flag at Gecko startup is a safe API, whereas deleting ProfLD/startupCaches in an Android intent handler is the definition of an unsafe API: prone to both racing and to code drift.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #9)
> (In reply to Richard Newman [:rnewman] from comment #8)
> > Comment on attachment 8412787 [details] [diff] [review]
> > Part 2: Purge startup caches when starting Gecko in developer builds.
> > r=mfinkle
> > 
> > Review of attachment 8412787 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I like the idea of fixing this pain point, but I have a number of questions
> > that I think are worth answering, if only for the record.
> > 
> > 1. Are you 100% convinced that MOZILLA_OFFICIAL is the right flag to use
> > here? Rather than, say, MOZ_UPDATE_CHANNEL.equals("default"), or a new flag?
> 
> No!  This is based on the discussions I have had with blassey in the
> Wednesday Mobile meeting.  I considered adding a config variable, etc, etc
> -- but it needs to default to on for devs, off for tbpl, etc, and it's
> irritating to add config variables for stuff that should be automatic.  I'd
> be happy to bump the logic to something like:

I'm fine with MOZ_OFFICIAL being the flag. I'd rather use it than MOZ_UPDATE_CHANNEL.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8412787 [details] [diff] [review]
Part 2: Purge startup caches when starting Gecko in developer builds. r=mfinkle

The in-tree mozconfigs do set MOZ_OFFICIAL so even TBPL and Nightlies will avoid the purge. I wanted to make sure that eideticker and other performance measurement systems would not be affected.

If we notice bad things, we can always back it out.

Wait for post-merge (Fx32 on trunk) to land so we have a long cycle to look for issues.

Comment 13

[Richard Newman [:rnewman]]

Thanks for the thorough answers, Nick!

Comment 14

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/9ba37373fa6c
https://hg.mozilla.org/integration/fx-team/rev/ed522d5b4d24

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ba37373fa6c
https://hg.mozilla.org/mozilla-central/rev/ed522d5b4d24