895028 - mobile/android/base/Makefile.in does not track buildid dependency

Status: NEW

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

While looking into bug 748470, I noticed that the Preprocessor commands use the buildid, but this dependency does not appear to be tracked by mobile/android/base/Makefile.in. (This is normally tracked - see webapprt/Makefile.in or toolkit/xre/Makefile.in for examples). If this dependency is added in, many commands are still executed during no-op builds even if the FORCE target is removed (which is the goal of 748470). In particular, this results in the following chain:

config/buildid -> AppConstants.java -> AppConstants.class -> gecko-browser.jar -> classes.dex

The .java -> .class command actually compiles ~500 .java files, which takes ~6 seconds on my machine. The classes.dex creation takes another ~8 seconds.

My concern is that even if we fix the Makefile to have all the correct dependencies, "no-op" builds will still do a lot of work. I have a few questions:

1) Is there a particular reason buildid does not need to be tracked in this directory?

2) Are there consequences if the buildid in this directory doesn't match the rest of the tree?

3) If we do need to track the buildid, can we avoid the expensive javac & classes.dex commands somehow?

Comment 1

[Michael Shal [:mshal]]

Attachment: graph of dependencies

I tried to generate a graph of dependencies coming from buildid for this directory - here's the dot file for it. With graphiz:

dot buildid.dot -Tpng > buildid.png

Not sure if it is completely accurate, but maybe it is helpful?

Comment 2

[(not currently active) Ted Mielczarek]

So this is the Java analogue of building the "firefox" binary on other platforms. On other platforms this works because nsBrowserApp.cpp does #include "application.ini.h":
http://mxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp#8

where that header is generated from application.ini:
http://mxr.mozilla.org/mozilla-central/source/build/Makefile.in#121

which in turn depends on the build id:
http://mxr.mozilla.org/mozilla-central/source/build/Makefile.in#16
http://mxr.mozilla.org/mozilla-central/source/build/Makefile.in#31

Comment 3

[Michael Shal [:mshal]]

Based on that it sounds like mobile/android/base/Makefile.in *should* be rebuilding things when buildid changes?

nalexander, can you comment on this?

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Michael Shal [:mshal] from comment #3)
> Based on that it sounds like mobile/android/base/Makefile.in *should* be
> rebuilding things when buildid changes?
> 
> nalexander, can you comment on this?

Yeah, unfortunately we do need to rebuild certain Java files when the buildid changes.  We really do use MOZ_APP_BUILDID in ANRReporter and FHR.  We could probably read the buildid from a json file to avoid the buildtime dep but I'd rather not.

Does the timestamp for buildid change every build?  (I think yes.)  One way we could minimize the rebuild time is to use newer Android toolchain pre-dexing.  What this means is that each jar is dexed, and then a cumulative dex is built.  (Right now, we dex the union of every jar every build.)  We could isolate the AppConstants (perhaps all preprocessed things?) in a very small jar that can be built and dexed quickly.

mshal, this confirms your concern that buildid changes will lead to non-noop builds, and suggests a way we might minimize this cost.  Happy to follow up.

Comment 5

[Nick Alexander :nalexander [he/him]]

Sorry, I responded and the flag was not cleared.

Comment 6

[Michael Shal [:mshal]]

(In reply to Nick Alexander :nalexander from comment #4)
> Does the timestamp for buildid change every build?  (I think yes.)  One way
> we could minimize the rebuild time is to use newer Android toolchain
> pre-dexing.  What this means is that each jar is dexed, and then a
> cumulative dex is built.  (Right now, we dex the union of every jar every
> build.)  We could isolate the AppConstants (perhaps all preprocessed
> things?) in a very small jar that can be built and dexed quickly.

The buildid changes everytime make enters the config/ directory, so if you do a build in a subdirectory (like obj/mobile/android/base), then the buildid won't be updated. I did confirm that even with the FORCE dependency present, things dependent on buildid are not updated correctly since the Preprocessor doesn't run (after an initial top-level 'make', config/buildid and AppConstants.java match. After a 2nd top-level no-op 'make', config/buildid is updated but AppConstants.java is not). So, I don't believe this will be made worse by the changes in bug 748470.

Building these into a smaller jar sounds like a good idea to me, though how are dependent java files handled? Ie: If the classes that go into gecko-browser.jar depend on AppConstants.class, won't rebuilding the small AppConstants jar still cause a recompile of gecko-browser.jar?

Comment 7

[Pete Moore [:pmoore][:pete]]

(In reply to Michael Shal [:mshal] from comment #6)

> Building these into a smaller jar sounds like a good idea to me, though how
> are dependent java files handled? Ie: If the classes that go into
> gecko-browser.jar depend on AppConstants.class, won't rebuilding the small
> AppConstants jar still cause a recompile of gecko-browser.jar?

Comment 8

[Nick Alexander :nalexander [he/him]]

Hey folks, I don't have much context of why this conversation has re-opened, and underlying Makefile.in has changed a good deal, but I'll respond inline.

(In reply to Michael Shal [:mshal] from comment #6)
> (In reply to Nick Alexander :nalexander from comment #4)
> > Does the timestamp for buildid change every build?  (I think yes.)  One way
> > we could minimize the rebuild time is to use newer Android toolchain
> > pre-dexing.  What this means is that each jar is dexed, and then a
> > cumulative dex is built.  (Right now, we dex the union of every jar every
> > build.)  We could isolate the AppConstants (perhaps all preprocessed
> > things?) in a very small jar that can be built and dexed quickly.
> 
> The buildid changes everytime make enters the config/ directory, so if you
> do a build in a subdirectory (like obj/mobile/android/base), then the
> buildid won't be updated. I did confirm that even with the FORCE dependency
> present, things dependent on buildid are not updated correctly since the
> Preprocessor doesn't run (after an initial top-level 'make', config/buildid
> and AppConstants.java match. After a 2nd top-level no-op 'make',
> config/buildid is updated but AppConstants.java is not). So, I don't believe
> this will be made worse by the changes in bug 748470.

OK, this is good: most folks rebuild by invoking |mach build mobile/android| or |mach build mobile/android/base|.  So we could depend on buildid and not screw everybody over.

> Building these into a smaller jar sounds like a good idea to me, though how
> are dependent java files handled? Ie: If the classes that go into
> gecko-browser.jar depend on AppConstants.class, won't rebuilding the small
> AppConstants jar still cause a recompile of gecko-browser.jar?

Right now we do build a tiny JAR with the preprocessed code, but it doesn't help: if JAR1 depends on JAR2 and JAR2 changes, JAR1 must be rebuilt.  (Build systems with Java knowledge, notably buck, do better here.)

It's worth mentioning that the real expensive dependency chain is:

config/buildid -> AndroidManifest.xml.in -> R.java -> gecko-browser.jar -> classes.dex

Changing R.java means rebuilding basically the entire app :(

Summing up: I am pretty happy to add an explicit dependency on buildid providing it doesn't change on |mach build mobile/android|.  Most Fennec developers are using IntelliJ and Gradle for some or all of their local work, which builds rather differently and would not be impacted.