optimizejars.py broken by the rename to omni.ja

RESOLVED FIXED in Firefox 11

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mak, Assigned: froydnj)

Tracking

(Blocks: 1 bug, {perf, regression})

13 Branch
mozilla13
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10 affected, firefox11+ fixed, firefox12+ fixed, firefox13+ fixed, firefox-esr1012+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
We don't optimize jars anymore, and builds ignore the problem, letting it regress.
(Reporter)

Updated

5 years ago
Keywords: regression

Comment 1

5 years ago
(In reply to Marco Bonardo [:mak] from comment #0)
> We don't optimize jars anymore, and builds ignore the problem, letting it
> regress.

Do we know what caused the regression?
(Reporter)

Comment 2

5 years ago
well, to begin with, at least a couple code points do jarfile.endswith(".jar")
not sure if there's other stuff involved at build config level

Comment 3

5 years ago
Try run for db2b291b5b8f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=db2b291b5b8f
Results (out of 1 total builds):
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tglek@mozilla.com-db2b291b5b8f

Comment 4

5 years ago
Try run for 8fcc594671a1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8fcc594671a1
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tglek@mozilla.com-8fcc594671a1
Assignee: nobody → taras.mozilla

Comment 5

5 years ago
I'm not working on this atm.
Assignee: taras.mozilla → nobody
status-firefox-esr10: --- → affected
status-firefox10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
tracking-firefox13: --- → ?
(Assignee)

Comment 6

5 years ago
I have some cycles to poke at this.
(Assignee)

Updated

5 years ago
Assignee: nobody → nfroyd
(Assignee)

Comment 7

5 years ago
Created attachment 597794 [details] [diff] [review]
patch

Here's the easy patch which makes optimizejars.py check for .ja extensions.  It would, of course, be nice to make this check more robust (check for zip file fingerprints?), but I do not know how fragile that would be.

Debugging output verified that we now look at omni.ja.
Attachment #597794 - Flags: review?(khuey)
Comment on attachment 597794 [details] [diff] [review]
patch

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

r=me assuming it works.
Attachment #597794 - Flags: review?(khuey) → review+
(Assignee)

Comment 9

5 years ago
How does one tell that it works, given that it took us a while to notice it was broken in the first place?
(Assignee)

Comment 10

5 years ago
Actually, going to assume that khuey didn't see my comment about debugging output.
Keywords: checkin-needed
Whiteboard: [autoland-try]

Updated

5 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]
(In reply to Nathan Froyd (:froydnj) from comment #10)
> Actually, going to assume that khuey didn't see my comment about debugging
> output.

That would be a correct assumption.

Comment 12

5 years ago
Autoland Patchset:
	Patches: 597794
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=49fd256eb14f
Try run started, revision 49fd256eb14f. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=49fd256eb14f

Comment 13

5 years ago
(In reply to Nathan Froyd (:froydnj) from comment #9)
> How does one tell that it works, given that it took us a while to notice it
> was broken in the first place?

you unzip onmi.ja and check that prefs.js is near the top(and unzip bitches about file being corrupt). Please don't land this until that's verified.
(Assignee)

Comment 14

5 years ago
(In reply to Taras Glek (:taras) from comment #13)
> (In reply to Nathan Froyd (:froydnj) from comment #9)
> > How does one tell that it works, given that it took us a while to notice it
> > was broken in the first place?
> 
> you unzip onmi.ja and check that prefs.js is near the top(and unzip bitches
> about file being corrupt). Please don't land this until that's verified.

With unzip -l, I see:

Archive:  dist/firefox/omni.ja
warning [dist/firefox/omni.ja]:  6747593 extra bytes at beginning or within zipfile
  (attempting to process anyway)
error [dist/firefox/omni.ja]:  reported length of central directory is
  -6747593 bytes too long (Atari STZip zipfile?  J.H.Holm ZIPSPLIT 1.1
  zipfile?).  Compensating...

which I assume is sufficient proof of corruption.

prefs.js, however, is nowhere near the top of the file.
(Assignee)

Comment 15

5 years ago
Created attachment 597874 [details]
unzip -l omni.ja output

See for yourself what things look like.
That doesn't look like it's been reordered at all.
My guess is that he doesn't have an ordering list. IIRC, it's generated during the PGO profile run.
(Assignee)

Comment 18

5 years ago
Created attachment 597887 [details]
unzip -l omni.ja output, optimized

Ah, I am enlightened.  So, after:

- starting dist/firefox/firefox with MOZ_JAR_LOG_DIR set;
- arranging for omni.ja.log to be deposited in jarlog/en-US/;
- running make package;

I get the attached output, which looks substantially different.
This does not affect the built product, so no need to track for a specific release. Once resolved, interested parties can grab the latest optimizejars.py.
tracking-firefox-esr10: ? → -
tracking-firefox11: ? → -
tracking-firefox12: ? → -
tracking-firefox13: ? → -
That evaluation is based upon https://bugzilla.mozilla.org/show_bug.cgi?id=726656#c0, which states that "We don't optimize jars anymore".
(In reply to Alex Keybl [:akeybl] from comment #20)
> That evaluation is based upon
> https://bugzilla.mozilla.org/show_bug.cgi?id=726656#c0, which states that
> "We don't optimize jars anymore".

That's the bug that this patch fixes.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #21)
> (In reply to Alex Keybl [:akeybl] from comment #20)
> > That evaluation is based upon
> > https://bugzilla.mozilla.org/show_bug.cgi?id=726656#c0, which states that
> > "We don't optimize jars anymore".
> 
> That's the bug that this patch fixes.

Yep - RyanVM clarified in #developers as well. I took "We don't optimize jars anymore" to mean this tool isn't necessary for builds.
tracking-firefox-esr10: - → 13+
tracking-firefox11: - → +
tracking-firefox12: - → +
tracking-firefox13: - → +

Comment 23

5 years ago
Try run for 49fd256eb14f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=49fd256eb14f
Results (out of 210 total builds):
    exception: 1
    success: 178
    warnings: 18
    failure: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-49fd256eb14f

Updated

5 years ago
Whiteboard: [autoland-in-queue]
(In reply to Mozilla RelEng Bot from comment #23)
> Results (out of 210 total builds):
>     exception: 1
^^^^^^^This is Jetpack (and hidden)

>     warnings: 18
^^^^^^^11 on OSX 10.7 that are hidden
^^^^^^^3 Android Native Opt that are hidden
^^^^^^^2 Intermittent
^^^^^^^1 Android Retriggered (Since log doesn't tell me much)
^^^^^^^1 Win Debug M(oth) that was leaking and I can't find a bug on, retriggered.

>     failure: 13
^^^^^^^These are all jetpack (and hidden)
(In reply to Alex Keybl [:akeybl] from comment #22)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #21)
> > (In reply to Alex Keybl [:akeybl] from comment #20)
> > > That evaluation is based upon
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=726656#c0, which states that
> > > "We don't optimize jars anymore".
> > 
> > That's the bug that this patch fixes.
> 
> Yep - RyanVM clarified in #developers as well. I took "We don't optimize
> jars anymore" to mean this tool isn't necessary for builds.

I'm guessing the tracking-esr10:13+ was a mistake and it should be 11+ given your choice to track this for 11 as well?
There won't be an ESR release based on 11.
(In reply to Justin Wood (:Callek) from comment #25)
> I'm guessing the tracking-esr10:13+ was a mistake and it should be 11+ given
> your choice to track this for 11 as well?

My expectation is that this will land on Mozilla 13 first. We'll change the tracking flag if approved for m-a or m-b.

(In reply to Ryan VanderMeulen from comment #26)
> There won't be an ESR release based on 11.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for info on how we track ESR changes.
(Reporter)

Updated

5 years ago
Blocks: 728176
(Reporter)

Comment 28

5 years ago
I have filed bug 728176 to investigate ways to fail the build when omni.ja optimization fails. So we don't mix up the two problems. Any idea for that should go there.
(Reporter)

Comment 29

5 years ago
https://hg.mozilla.org/mozilla-central/rev/f88a05e00f47
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
Please nominate for approval Aurora/Beta approval if you believe this fix to be low risk (sounds like it).

Updated

5 years ago
status-firefox13: affected → ---
(Assignee)

Comment 31

5 years ago
Comment on attachment 597794 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 701875
User impact if declined: Slower startup.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Epsilon.
String changes made by this patch: None
Attachment #597794 - Flags: approval-mozilla-beta?
Attachment #597794 - Flags: approval-mozilla-aurora?
Comment on attachment 597794 [details] [diff] [review]
patch

[Triage Comment]
Approved for Aurora 12 and Beta 11.
Attachment #597794 - Flags: approval-mozilla-beta?
Attachment #597794 - Flags: approval-mozilla-beta+
Attachment #597794 - Flags: approval-mozilla-aurora?
Attachment #597794 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 33

5 years ago
Comment on attachment 597794 [details] [diff] [review]
patch

Need this checked in to aurora and beta; if somebody would be so kind as to tack a=akeybl on the commit line, that would be appreciated.  Thanks!
Attachment #597794 - Flags: checkin?
(Reporter)

Comment 34

5 years ago
I can do that.
(Reporter)

Comment 35

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/fd7744b11e51
https://hg.mozilla.org/releases/mozilla-beta/rev/db6414deacea
status-firefox11: affected → fixed
status-firefox12: affected → fixed
Depends on: 730196
(Assignee)

Updated

5 years ago
Attachment #597794 - Flags: checkin? → checkin+
Whiteboard: [qa-]
We didn't change the ESR tracking flag from 13+ to 11+ when this landed a month ago. Feel free to land an appropriate patch on the ESR branch whenever possible.
status-firefox13: --- → fixed
tracking-firefox-esr10: 13+ → 12+
(Assignee)

Comment 37

5 years ago
http://hg.mozilla.org/releases/mozilla-esr10/rev/08ac106b5072
status-firefox-esr10: affected → fixed
You need to log in before you can comment on or make changes to this bug.