633645 - Add startup cache to omnijar during make package

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Michael Wu [:mwu]]

Attachment: Add startup cache to omnijar during make package

We can generate an omnijar startup cache during make package instead of during PGO. This patch also improves the startup cache generation script to cover more js.

Comment 1

[Michael Wu [:mwu]]

Attachment: 1. Improve precompile_cache.js

1. Keep style consistent.
2. Remove zip extraction code. This is better done outside the script. Also, store the startupcache to something other than omni.jar so we can use infozip to add the files.
3. Make the script precompile every js file with the appropriate URI. I checked that the profile startupcache didn't have any js(m).bin that was shipped with omni.jar.

Comment 2

[Michael Wu [:mwu]]

Attachment: 2. Hook up precompile_cache.js (WIP)

We hook in with GENERATE_CACHE to ensure that cache generation is only done when packaging is done from browser/installer. No cache generation should be done during locale repack.

Comment 3

[Michael Wu [:mwu]]

Attachment: 1. Improve precompile_cache.js, v2

Avoid duplicating sync js in the startup cache.

Comment 4

[Michael Wu [:mwu]]

Attachment: Bonus patch: Remove original js files

This patch removes js components/modules which have been cached.

Comment 5

[Michael Wu [:mwu]]

Attachment: 2. Hook up precompile_cache.js, v2

Well, it works. Not pretty but it gives us pregenerated startup caches for all desktop platforms. I think we may want some sort of zip stripping utility so we don't have to manually set the timestamps and use zip -X.

Comment 6

[Michael Wu [:mwu]]

Attachment: 1. Improve precompile_cache.js, v3

Comment 7

[(dormant account)]

Comment on attachment 514236 [details] [diff] [review]
1. Improve precompile_cache.js, v3

> function load_entries(entries, prefix) {
>-  while(entries.hasMore()) {
>+  while (entries.hasMore()) {
>     var c = entries.getNext();
>+    if (c.indexOf("services-sync") >= 0)
>+      continue;
>+    if (c.indexOf("services-crypto") >= 0)
>+      continue;
>     load(prefix + c);
>   }
> }

This needs a comment that this is for skipping sync urls which will be caught elsewhere.

Comment 8

[Michael Wu [:mwu]]

Attachment: 2. Hook up precompile_cache.js, v3

Added a small change to make this work on windows.

Comment 9

[Mike Hommey [:glandium]]

Does that work well with bug 620931 ?

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 515715 [details] [diff] [review]
2. Hook up precompile_cache.js, v3

>diff --git a/browser/installer/Makefile.in b/browser/installer/Makefile.in
>--- a/browser/installer/Makefile.in
>+++ b/browser/installer/Makefile.in
>+GENERATE_CACHE = \
>+  $(_ABS_RUN_TEST_PROGRAM) $(_ABS_DIST)/bin/xpcshell$(BIN_SUFFIX) -g "$$PWD" $(topsrcdir)/browser/installer/precompile_cache.js && \

You should be able to use $(CURDIR) instead of $$PWD.

>+  rm -rf jsloader && \
>+  $(UNZIP) startupCache.zip && \

Can you make startupCache.zip a variable, and pass it into precompile_cache.js?

>+  rm startupCache.zip && \
>+  find jsloader | xargs touch -t 201001010000 && \
>+  $(ZIP) -r9mX omni.jar jsloader
>+endif

This whole block is pretty awful. I don't know have to make it entirely better. It'd be nice to at least get rid of that find | touch bit, and having the extra zip with all the goofy options there seems weird. (Plus, any other app that wants to do this is going to wind up duplicating all this stuff, too.)

>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>--- a/toolkit/mozapps/installer/packager.mk
>+++ b/toolkit/mozapps/installer/packager.mk
>@@ -297,6 +297,8 @@ MAKE_SDK = $(CREATE_FINAL_TAR) - $(MOZ_A
> endif
> 
> ifdef MOZ_OMNIJAR
>+GENERATE_CACHE ?= echo

Maybe just 'true'?

r+ because I don't really have a better idea for what you're doing, but I would like something better there.

Comment 11

[Michael Wu [:mwu]]

(In reply to comment #10)
> >+  rm startupCache.zip && \
> >+  find jsloader | xargs touch -t 201001010000 && \
> >+  $(ZIP) -r9mX omni.jar jsloader
> >+endif
> 
> This whole block is pretty awful. I don't know have to make it entirely better.
> It'd be nice to at least get rid of that find | touch bit, and having the extra
> zip with all the goofy options there seems weird. (Plus, any other app that
> wants to do this is going to wind up duplicating all this stuff, too.)
> 
I'll clean this up in another bug.

Comment 12

[(not currently active) Ted Mielczarek]

catlee is going to rewrite most of packager.mk in Python, this could wind up in there somehow.

Comment 13

[Michael Wu [:mwu]]

(In reply to comment #10)
> Comment on attachment 515715 [details] [diff] [review]
> 2. Hook up precompile_cache.js, v3
> 
> >diff --git a/browser/installer/Makefile.in b/browser/installer/Makefile.in
> >--- a/browser/installer/Makefile.in
> >+++ b/browser/installer/Makefile.in
> >+GENERATE_CACHE = \
> >+  $(_ABS_RUN_TEST_PROGRAM) $(_ABS_DIST)/bin/xpcshell$(BIN_SUFFIX) -g "$$PWD" $(topsrcdir)/browser/installer/precompile_cache.js && \
> 
> You should be able to use $(CURDIR) instead of $$PWD.
> 

CURDIR is the directory that make starts in, right? We're trying to run in the staging directory, so I don't think $(CURDIR) would be right here. I think I used $$PWD instead of . here so we can get an absolute path.

Comment 14

[(not currently active) Ted Mielczarek]

Oh, good point. You're inside some awful mess of cd whatever && do something else, aren't you? In that case, yeah, carry on with the awfulness.

Comment 15

[Michael Wu [:mwu]]

Comment about passing startupCache.zip into precompile_cache.js and s/echo/true/ addressed.

http://hg.mozilla.org/projects/build-system/rev/024a089c274d
http://hg.mozilla.org/projects/build-system/rev/a09be7210d46

Comment 16

[(not currently active) Ted Mielczarek]

This wasn't quite what I meant:
http://hg.mozilla.org/projects/build-system/rev/a09be7210d46#l1.17

I meant putting the filename into a makefile variable that you could reference there, since you're still repeating it. That is an improvement over repeating it a third time in the script, though.

Comment 17

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/024a089c274d
http://hg.mozilla.org/mozilla-central/rev/a09be7210d46

Still planning to follow up on this stuff and clean it up a bit, but I'm distracted with other stuff right now.

Comment 18

[Landry Breuil (:gaston)]

For the record, it seems those commits broke make package on sparc64, see http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/27.
build 26 on may 17 was fine, and since that date make package fails as can be seen on http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64.

Since we run more js during make package, it shows how much js/xpcshell is broken on sparc64...

Comment 19

[Michael Wu [:mwu]]

(In reply to comment #18)
> For the record, it seems those commits broke make package on sparc64, see
> http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/27.
> build 26 on may 17 was fine, and since that date make package fails as can
> be seen on http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64.
> 
> Since we run more js during make package, it shows how much js/xpcshell is
> broken on sparc64...

You could avoid running this bit of JS during packaging by modifying this line http://hg.mozilla.org/mozilla-central/file/b90cbc558b36/browser/installer/Makefile.in#l120 to not run on sparc64, but yeah, xpcshell being broken is an even bigger problem.