559961 - Reorder jar files as part of PGO

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(dormant account)]

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.

Comment 1

[(dormant account)]

Attachment: tool to reorder jars

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

Comment 2

[(dormant account)]

Attachment: proof of concept: jar logging + index-at-front shortcut

Comment 3

[(dormant account)]

Attachment: complete patch

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.

Comment 5

[Mike Hommey [:glandium]]

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?

Comment 6

[(dormant account)]

(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.

Comment 7

[(dormant account)]

Attachment: This should order jars on PGO

Comment 8

[(dormant account)]

Attachment: order omnijar

Comment 9

[(dormant account)]

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.

Comment 10

[(dormant account)]

Looks like my profileserver.py change wasn't right. This doesn't quite generate logs during pgo(at least according to try server)

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 12

[(dormant account)]

We don't reorder on pgo, the logs are supposed to be gathered during pgo. They are then consulted during make package.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 14

[Mike Hommey [:glandium]]

(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.)

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't see why we can't fire up the browser (or whatever) at packaging time.

Comment 16

[Mike Hommey [:glandium]]

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).

Comment 17

[(dormant account)]

Attachment: order jars on pgo

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.

Comment 18

[Benjamin Smedberg]

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.

Comment 19

[(dormant account)]

Attachment: order jars on pgo [Checkin: Comment 28]

Addressed review comment. Carrying over r+

Comment 20

[(dormant account)]

This should block ff4 as it's a significant perf boost for omnijar.

Comment 21

[Michael Wu [:mwu]]

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.

Comment 22

[Benjamin Smedberg]

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?

Comment 23

[Mike Hommey [:glandium]]

(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.

Comment 24

[(dormant account)]

(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.

Comment 25

[Benjamin Smedberg]

Can you please file a releng bug to that effect?

Comment 26

[Robert Kaiser]

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

Comment 27

[Takanori MATSUURA]

In xulrunner/app/nsRegisterGREUnix.cpp:

#include <mozilla/FileUtils.h>
should be
#include "mozilla/FileUtils.h"

Comment 28

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/32d403bf5a7a
http://hg.mozilla.org/mozilla-central/rev/dde9bc692553

Comment 29

[Takanori MATSUURA]

Attachment: fix?

To fix compilation failure on Fedora 13 x86_64 at xulrunner/app/nsRegisterGREUnix.cpp

Comment 30

[Axel Hecht [:Pike]]

Attachment: abspath topsrcdir [Checkin: Comment 32]

The l10n repacks run in the srcdir, which makes topsrcdir a relative path. Explicitly core_abspath that.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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?

Comment 32

[Axel Hecht [:Pike]]

Comment on attachment 467405 [details] [diff] [review]
abspath topsrcdir [Checkin: Comment 32]

Landed, http://hg.mozilla.org/mozilla-central/rev/a7874c76431f

Comment 33

[Robert Kaiser]

(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.

Comment 34

[Axel Hecht [:Pike]]

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.

Comment 35

[Axel Hecht [:Pike]]

Attachment: doh, js, too :-( [Checkin: Comment 52]

Comment 37

[Robert Kaiser]

Attachment: do the abspath trick on c-c as well [Checkin: Comment 38]

The abspath trick is needed on comm-central as well for L10n repacks, in that case.

Comment 38

[Robert Kaiser]

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

Comment 39

[Takanori MATSUURA]

(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.

Comment 40

[[:fabrice] Fabrice Desré]

(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.

Comment 41

[Takanori MATSUURA]

Attachment: fix? v2

Update my patch with merging attachment 467417 [details] [diff] [review] in bug 588783.

Comment 43

[(dormant account)]

(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?

Comment 44

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 45

[Takanori MATSUURA]

(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].

Comment 46

[(dormant account)]

(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.

Comment 47

[Brendan Eich [:brendan]]

(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

Comment 48

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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!

Comment 49

[Takanori MATSUURA]

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.

Comment 50

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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 ;-)

Comment 51

[Takanori MATSUURA]

(In reply to comment #50)
Oh, thanks!

Comment 52

[Takanori MATSUURA]

(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

Comment 53

[(dormant account)]

Pushed Takanori's fix.

http://hg.mozilla.org/mozilla-central/rev/f0af50ab4215

Comment 54

[Mike Hommey [:glandium]]

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.

Comment 55

[Ginn Chen]

I'm curious, too. It happened several times, but not every time.
I'll be more caution. Sorry for the spam.

Comment 56

[Jerry Baker]

(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.

Comment 57

[Reuben Morais [:reuben]]

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)