Closed Bug 633645 Opened 9 years ago Closed 9 years ago

Add startup cache to omnijar during make package

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mwu, Assigned: mwu)

References

Details

(Whiteboard: fixed-in-bs)

Attachments

(3 files, 5 obsolete files)

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.
Attached patch 1. Improve precompile_cache.js (obsolete) — Splinter Review
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.
Attachment #511869 - Attachment is obsolete: true
Attachment #512274 - Flags: review?(tglek)
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.
Attachment #512274 - Flags: review?(tglek) → review+
Avoid duplicating sync js in the startup cache.
Attachment #512274 - Attachment is obsolete: true
This patch removes js components/modules which have been cached.
Severity: normal → enhancement
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.
Attachment #512278 - Attachment is obsolete: true
Attachment #514234 - Flags: review?(ted.mielczarek)
Attachment #512320 - Attachment is obsolete: true
Attachment #514236 - Flags: review?(tglek)
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.
Attachment #514236 - Flags: review?(tglek) → review+
Added a small change to make this work on windows.
Attachment #514234 - Attachment is obsolete: true
Attachment #515715 - Flags: review?(ted.mielczarek)
Attachment #514234 - Flags: review?(ted.mielczarek)
Does that work well with bug 620931 ?
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.
Attachment #515715 - Flags: review?(ted.mielczarek) → review+
(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.
catlee is going to rewrite most of packager.mk in Python, this could wind up in there somehow.
(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.
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 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
Whiteboard: fixed-in-bs
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.
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 652517
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...
(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.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.