Closed Bug 559961 Opened 11 years ago Closed 10 years ago

Reorder jar files as part of PGO

Categories

(Firefox Build System :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(blocking2.0 beta5+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

(Whiteboard: [ts])

Attachments

(5 files, 7 obsolete files)

The build process should be able to notify firefox when executing a build for pgo purposes. Then the jar component can output jar-file-order info and jars could be repacked on the second build pass.
Whiteboard: [ts]
Attached file tool to reorder jars (obsolete) —
This is an extension of mwu's hack. It reorders the jar index(for easier debugging) and places the index at the front of the file
Assignee: nobody → tglek
Attachment #462938 - Attachment mime type: text/x-csrc → text/plain
Attached patch complete patch (obsolete) — Splinter Review
Rewrote the C tool in python so it integrated better into build system. All that is left is for pgo to set MOZ_JAR_LOG_DIR=/dist/jarlog/ to turn this on.
Attachment #462938 - Attachment is obsolete: true
Attachment #462953 - Attachment is obsolete: true
Duplicate of this bug: 582061
Blocks: 556644
If that's going to be part of pgo, wouldn't it be better if the code generating the log was #ifdef'ed, so that it is not built on the second pass?
(In reply to comment #5)
> If that's going to be part of pgo, wouldn't it be better if the code generating
> the log was #ifdef'ed, so that it is not built on the second pass?

No, I'd also like extensions to use this as a profiling mechanism so they can ship optimized jars.
Attached patch This should order jars on PGO (obsolete) — Splinter Review
Attachment #464161 - Attachment is obsolete: true
Attached patch order omnijar (obsolete) — Splinter Review
Attachment #464974 - Flags: review?(me)
Comment on attachment 464974 [details] [diff] [review]
This should order jars on PGO

This patch orders jars according to how they are used. This also deviates from the typical jar layout by sticking the central directory at the front(the jar is still backwards compatible with other zip tools).
With these two changes jars can now be read in a linear pattern yielding over 2x reduction in pagefaults.
Attachment #464974 - Flags: review?(benjamin)
Looks like my profileserver.py change wasn't right. This doesn't quite generate logs during pgo(at least according to try server)
Comment on attachment 464974 [details] [diff] [review]
This should order jars on PGO

So, looking through at this, the one thing that I don't understand is why we need to do this during PGO.  Can we do this at packaging time?  It seems to me that you could build an instrumented version of libjar and drop that in a build and start it up and gather the information there.

If it is done as part of PGO this raises all sorts of hairy l10n repack issues.
Attachment #464974 - Flags: review?(me)
We don't reorder on pgo, the logs are supposed to be gathered during pgo. They are then consulted during make package.
(In reply to comment #12)
> We don't reorder on pgo, the logs are supposed to be gathered during pgo. They
> are then consulted during make package.

My point is that ideally we'd gather the logs during packaging because
- It really has no relevance to the actual compiler PGO
- And doing the logging at packaging time will make it much easier for localized builds to benefit.
(In reply to comment #13)
> My point is that ideally we'd gather the logs during packaging because
> - It really has no relevance to the actual compiler PGO
> - And doing the logging at packaging time will make it much easier for
> localized builds to benefit.

AFAIU, you just can't gather the logs during packaging, because you need to know in what order the files are accessed *at runtime*. Normal packaging doesn't run anything.

What could be done, though, is to have an order file in the build tree, and have the PGO script check that it's not too far from the actual order. The file could then occasionally be updated in the tree, when it makes sense (before a release, after massive changes, etc.)
I don't see why we can't fire up the browser (or whatever) at packaging time.
Well, for one, because not everybody building the browser has the possibility to do so (think linux autobuilders without X11 ; though if they are going to run pgo or reftest/mochitest, they'll have to, but not everybody does).
Attached patch order jars on pgo (obsolete) — Splinter Review
I would like to land this post-beta4 to get some testing in the nightlies. This works with or without omnijar.
Reason logs are gathered during pgo is because it's tricky to run the browser during packaging. Since pgo already runs the browser, it's a natural place to get a "natural" trace of our io patterns.
I'd like to tackle l10n repacks in a separate bug so this code can land and get tested on our nightlies prior to b5.

I think the code here is pretty simple. I'm mainly curious whether optimizejars.py should go elsewhere in the tree.
Attachment #464974 - Attachment is obsolete: true
Attachment #464975 - Attachment is obsolete: true
Attachment #466399 - Flags: review?(benjamin)
Attachment #464974 - Flags: review?(benjamin)
Comment on attachment 466399 [details] [diff] [review]
order jars on pgo

style nit, please add a newline after "namespace mozilla {" and before the closing namespace brace.

Also "} // namespace mozilla" for clarity.
Attachment #466399 - Flags: review?(benjamin) → review+
Addressed review comment. Carrying over r+
Attachment #466399 - Attachment is obsolete: true
Attachment #466737 - Flags: review+
This should block ff4 as it's a significant perf boost for omnijar.
blocking2.0: --- → ?
Comment on attachment 466737 [details] [diff] [review]
order jars on pgo [Checkin: Comment 28]

I believe we need this patch to take full advantage of omnijar.
Attachment #466737 - Flags: approval2.0?
blocking2.0: ? → beta5+
We don't actually do PGO runs on non-Windows, right? How are you going to actually get this hooked up with Linux/Mac builds?
(In reply to comment #22)
> We don't actually do PGO runs on non-Windows, right? How are you going to
> actually get this hooked up with Linux/Mac builds?

It looks like Taras is trying to get at least Linux builds do PGO for 4.0, and from the bugs I'm following it looks like it may happen. No idea for Mac, but it would still be a good idea imho to implement something like the 2nd paragraph in comment 14.
(In reply to comment #22)
> We don't actually do PGO runs on non-Windows, right? How are you going to
> actually get this hooked up with Linux/Mac builds?

We'll get Linux as soon as we can deploy GCC 4.5, bug 559964. Not sure what the build-team holdup there is atm.

On Mac we can start running pgo script just for this if we deem this to be important.
Attachment #466737 - Flags: approval2.0?
Can you please file a releng bug to that effect?
Pushed the config.mk fix to comm-central as well, looks like we had busted packaging due to the var not existing. http://hg.mozilla.org/comm-central/rev/9c4c87dd8bc9
In xulrunner/app/nsRegisterGREUnix.cpp:

#include <mozilla/FileUtils.h>
should be
#include "mozilla/FileUtils.h"
Attached patch fix?Splinter Review
To fix compilation failure on Fedora 13 x86_64 at xulrunner/app/nsRegisterGREUnix.cpp
Attachment #467378 - Flags: review?(tglek)
The l10n repacks run in the srcdir, which makes topsrcdir a relative path. Explicitly core_abspath that.
Component: Networking: JAR → Build Config
QA Contact: networking.jar → build-config
(In reply to comment #29)
> Created attachment 467378 [details] [diff] [review]
> fix?
> 
> To fix compilation failure on Fedora 13 x86_64 at
> xulrunner/app/nsRegisterGREUnix.cpp

Takanori, is the using namespace mozilla; required?  Can we qualify the names instead?
(In reply to comment #32)
> Landed, http://hg.mozilla.org/mozilla-central/rev/a7874c76431f

1) I think you'll need to apply that change in js/ as well or things will break,
2) Could you apply that core_abspath change to comm-central/config/config.mk as well? rs=me for that.

Thanks.
I did check js/src/config.mk, that looks pretty different already and doesn't have optimizejars.py at all.

And I haven't pushed to comm-central in ages, no idea what the infrastructure for that looks like these days. I'd rather have someone else patch and push there.
Attachment #467378 - Flags: review?(tglek) → review+
Duplicate of this bug: 588783
The abspath trick is needed on comm-central as well for L10n repacks, in that case.
Attachment #467458 - Flags: review?(bugspam.Callek)
Attachment #467458 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 467458 [details] [diff] [review]
do the abspath trick on c-c as well [Checkin: Comment 38]

Pushed as http://hg.mozilla.org/comm-central/rev/6689958f1d25
(In reply to comment #31)
> (In reply to comment #29)
> Takanori, is the using namespace mozilla; required?  Can we qualify the names
> instead?

As per bug 588783, "using namespace mozilla" is required.
(In reply to comment #39)
> (In reply to comment #31)
> > (In reply to comment #29)
> > Takanori, is the using namespace mozilla; required?  Can we qualify the names
> > instead?
> 
> As per bug 588783, "using namespace mozilla" is required.

Actually the patch in bug 588783 uses qualified names.
Attached patch fix? v2 (obsolete) — Splinter Review
Update my patch with merging attachment 467417 [details] [diff] [review] in bug 588783.
Attachment #467378 - Attachment is obsolete: true
Attachment #467617 - Flags: review?(tglek)
Depends on: 589153
Duplicate of this bug: 589153
(In reply to comment #41)
> Created attachment 467617 [details] [diff] [review]
> fix? v2
> 
> Update my patch with merging attachment 467417 [details] [diff] [review] in bug 588783.

Why did you decide to not do using namespace mozilla?
(In reply to comment #43)
> (In reply to comment #41)
> > Created attachment 467617 [details] [diff] [review] [details]
> > fix? v2
> > 
> > Update my patch with merging attachment 467417 [details] [diff] [review] [details] in bug 588783.
> Why did you decide to not do using namespace mozilla?

Probably because of comment 31. :-P

Did we ever establish good rules on using c++ namespaces.  I remember .platform threads on them but never any concrete rules.
(In reply to comment #44)
> Probably because of comment 31. :-P

Yes. and comment #40.

I don't know which is the better approach to fix, attachment 467378 [details] [diff] [review] or attachment 467617 [details] [diff] [review].
(In reply to comment #45)
> (In reply to comment #44)
> > Probably because of comment 31. :-P
> 
> Yes. and comment #40.
> 
> I don't know which is the better approach to fix, attachment 467378 [details] [diff] [review] or
> attachment 467617 [details] [diff] [review].

Neither of those seems like a good reason. I already r+ed a using namespace mozilla patch, that should be enough.
(In reply to comment #44)
> (In reply to comment #43)
> > (In reply to comment #41)
> > > Created attachment 467617 [details] [diff] [review] [details] [details]
> > > fix? v2
> > > 
> > > Update my patch with merging attachment 467417 [details] [diff] [review] [details] [details] in bug 588783.
> > Why did you decide to not do using namespace mozilla?
> 
> Probably because of comment 31. :-P

Circular argument! :-(

> Did we ever establish good rules on using c++ namespaces.  I remember .platform
> threads on them but never any concrete rules.

The rules are being followed all the time. Don't put using namespace foo; in a .h file. Do use it in .cpp files to avoid :: eyesores. What's the question?

/be
(In reply to comment #47)
> (In reply to comment #44)
> > (In reply to comment #43)
> > > (In reply to comment #41)
> > > > Created attachment 467617 [details] [diff] [review] [details] [details] [details]
> > > > fix? v2
> > > > 
> > > > Update my patch with merging attachment 467417 [details] [diff] [review] [details] [details] [details] in bug 588783.
> > > Why did you decide to not do using namespace mozilla?
> > 
> > Probably because of comment 31. :-P
> Circular argument! :-(

My point exactly.

> > Did we ever establish good rules on using c++ namespaces.  I remember .platform
> > threads on them but never any concrete rules.
> The rules are being followed all the time. Don't put using namespace foo; in a
> .h file. Do use it in .cpp files to avoid :: eyesores. What's the question?
> /be

That was my question.  Thanks!
Blocks: 589368
Thank you everyone.

I'd like to choose attachment 467378 [details] [diff] [review].
Do I have to re-apply attachment 467378 [details] [diff] [review] here?

I have no way to cancel obsolete flag.
Attachment #467617 - Attachment is obsolete: true
Attachment #467617 - Flags: review?(tglek)
Keywords: checkin-needed
Whiteboard: [ts] → [ts][c-n https://bugzilla.mozilla.org/attachment.cgi?id=467378]
(In reply to comment #49)
> Thank you everyone.
> 
> I'd like to choose attachment 467378 [details] [diff] [review].
> Do I have to re-apply attachment 467378 [details] [diff] [review] here?
> 
> I have no way to cancel obsolete flag.

You have to click on Details, then the Edit link.  Hardly user-friendly ;-)
(In reply to comment #50)
Oh, thanks!
Attachment #466737 - Attachment description: order jars on pgo → order jars on pgo [Checkin: Comment 28]
Attachment #467405 - Attachment description: abspath topsrcdir → abspath topsrcdir [Checkin: Comment 32]
Attachment #467458 - Attachment description: do the abspath trick on c-c as well → do the abspath trick on c-c as well [Checkin: Comment 38]
(In reply to comment #35)
> Created attachment 467411 [details] [diff] [review]
> doh, js, too :-(

I found this has been already applied
http://hg.mozilla.org/mozilla-central/rev/7fe781556c55
Attachment #467411 - Attachment description: doh, js, too :-( → doh, js, too :-( [Checkin: Comment 52]
Pushed Takanori's fix.

http://hg.mozilla.org/mozilla-central/rev/f0af50ab4215
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [ts][c-n https://bugzilla.mozilla.org/attachment.cgi?id=467378] → [ts]
Keywords: checkin-needed
OS: Linux → Windows CE
Ginn, I don't know what's wrong with your setup, but everytime you touch a bug in any way, you're changing the OS to WinCE.
OS: Windows CE → Linux
I'm curious, too. It happened several times, but not every time.
I'll be more caution. Sorry for the spam.
(In reply to comment #9)
> Comment on attachment 464974 [details] [diff] [review]
> This patch orders jars according to how they are used. This also deviates from
> the typical jar layout by sticking the central directory at the front(the jar
> is still backwards compatible with other zip tools).

This is NOT compatible with some other archive applications. This breaks 7zip - a widely used free Windows archive tool. See https://sourceforge.net/tracker/?func=detail&aid=3065694&group_id=14481&atid=114481.
Blocks: 619066
Blocks: 641614
This also breaks jar:

/Applications/Nightly.app/Contents/MacOS $ jar tf omni.jar 
java.util.zip.ZipException: invalid CEN header (bad signature)
	at java.util.zip.ZipFile.open(Native Method)
	at java.util.zip.ZipFile.<init>(ZipFile.java:127)
	at java.util.zip.ZipFile.<init>(ZipFile.java:88)
	at sun.tools.jar.Main.list(Main.java:979)
	at sun.tools.jar.Main.run(Main.java:224)
	at sun.tools.jar.Main.main(Main.java:1149)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.