Closed Bug 589971 Opened 10 years ago Closed 10 years ago

Omnijar before profiling part of PGO

Categories

(Firefox Build System :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(blocking2.0 final+)

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: taras.mozilla, Assigned: mwu)

References

Details

(Whiteboard: [ts])

Attachments

(1 file, 3 obsolete files)

Currently omnijaring happens at packaging which means optimized jar ordering from bug 559961 isn't getting triggered.
blocking2.0: --- → ?
559961 blocks, and this basically prevents it from being used, so this should definitely block.
Blocks: 559961
We could ostensibly run stage-package before profiling, and then run the binary from the staging directory. We'd need to be mindful of where the profiling data winds up, though. On Windows we assume it winds up in dist/bin right now:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#971

I think GCC just writes them next to the object files or something, so it may not matter there.
Yeah, we should profile packaged builds: the startup perf is probably different-enough that we might not PGO the JAR-loading bits correctly.
blocking2.0: ? → final+
Attached patch Package before running PGO (obsolete) — Splinter Review
This seems to do the trick. All green on windows try.
Attachment #470490 - Flags: review?(benjamin)
Attachment #470490 - Flags: review?(benjamin) → review?(ted.mielczarek)
Comment on attachment 470490 [details] [diff] [review]
Package before running PGO

>diff --git a/build/automation-build.mk b/build/automation-build.mk
>--- a/build/automation-build.mk
>+++ b/build/automation-build.mk
>@@ -16,7 +16,7 @@ else
> browser_path = \"$(TARGET_DIST)/$(MOZ_APP_DISPLAYNAME).app/Contents/MacOS/$(PROGRAM)\"
> endif
> else
>-browser_path = \"$(TARGET_DIST)/bin/$(PROGRAM)\"
>+browser_path = \"$(TARGET_DIST)/$(MOZ_APP_NAME)/$(PROGRAM)\"

This is going to break running Mochitest etc. from the objdir. I think you can probably just override this after including automation-build.mk here:
http://mxr.mozilla.org/mozilla-central/source/build/pgo/Makefile.in

while you're there, could you fix the OS X case as well so if we ever want to do PGO builds there, they'll work?

Looks fine otherwise.
Attachment #470490 - Flags: review?(ted.mielczarek) → review-
(In reply to comment #5)
> Comment on attachment 470490 [details] [diff] [review]
> Package before running PGO
> 
> >diff --git a/build/automation-build.mk b/build/automation-build.mk
> >--- a/build/automation-build.mk
> >+++ b/build/automation-build.mk
> >@@ -16,7 +16,7 @@ else
> > browser_path = \"$(TARGET_DIST)/$(MOZ_APP_DISPLAYNAME).app/Contents/MacOS/$(PROGRAM)\"
> > endif
> > else
> >-browser_path = \"$(TARGET_DIST)/bin/$(PROGRAM)\"
> >+browser_path = \"$(TARGET_DIST)/$(MOZ_APP_NAME)/$(PROGRAM)\"
> 
> This is going to break running Mochitest etc. from the objdir. I think you can
> probably just override this after including automation-build.mk here:
> http://mxr.mozilla.org/mozilla-central/source/build/pgo/Makefile.in
> 
Unfortunately. profileserver.py.in doesn't get its value directly from $browser_path. It gets it from automation.py.in. I'll try to find another way.
Right, but where do you think automation.py.in gets it?
http://mxr.mozilla.org/mozilla-central/source/build/automation-build.mk#29
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#78

Everything that uses automation.py gets its own copy, so if you override that in the build/pgo Makefile you should be fine.
Attached patch Package before running PGO, v2 (obsolete) — Splinter Review
Done as the review specified. Builds from the try server available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/mwu@mozilla.com-254e2ea5f9a0 As far as I can tell, the reordering was successful.
Attachment #470490 - Attachment is obsolete: true
Attachment #473372 - Flags: review?(ted.mielczarek)
Comment on attachment 473372 [details] [diff] [review]
Package before running PGO, v2

># HG changeset patch
># User Michael Wu <mwu@mozilla.com>
># Parent 59496a0802f7802056a1b279573666615fbd96f8
>Bug 589971 - Omnijar before profiling part of PGO, try: --build o --p win32 --m none --u all --t all
>
>diff --git a/build/automation-build.mk b/build/automation-build.mk
>--- a/build/automation-build.mk
>+++ b/build/automation-build.mk
>@@ -12,11 +12,14 @@ else
> ifeq ($(OS_ARCH),Darwin)
> ifdef MOZ_DEBUG
> browser_path = \"$(TARGET_DIST)/$(MOZ_APP_DISPLAYNAME)Debug.app/Contents/MacOS/$(PROGRAM)\"
>+packaged_path = \"$(TARGET_DIST)/$(MOZ_APP_NAME)/$(MOZ_APP_DISPLAYNAME)Debug.app/Contents/MacOS/$(PROGRAM)\"
> else
> browser_path = \"$(TARGET_DIST)/$(MOZ_APP_DISPLAYNAME).app/Contents/MacOS/$(PROGRAM)\"
>+packaged_path = \"$(TARGET_DIST)/$(MOZ_APP_NAME)/$(MOZ_APP_DISPLAYNAME).app/Contents/MacOS/$(PROGRAM)\"
> endif
> else
> browser_path = \"$(TARGET_DIST)/bin/$(PROGRAM)\"
>+packaged_path = \"$(TARGET_DIST)/$(MOZ_APP_NAME)/$(PROGRAM)\"
> endif
> endif
> 
>@@ -27,6 +30,7 @@ _CERTS_SRC_DIR = $(ABSOLUTE_TOPSRCDIR)/b
> 
> AUTOMATION_PPARGS = 	\
> 			-DBROWSER_PATH=$(browser_path) \
>+			-DPACKAGED_PATH=$(packaged_path) \
> 			-DXPC_BIN_PATH=\"$(LIBXUL_DIST)/bin\" \
> 			-DBIN_SUFFIX=\"$(BIN_SUFFIX)\" \
> 			-DPROFILE_DIR=\"$(_PROFILE_DIR)\" \

Can you detabify this down to a two-space indent while you're here?

Looks fine otherwise.
Attachment #473372 - Flags: review?(ted.mielczarek) → review+
Attached patch Package before running PGO, v3 (obsolete) — Splinter Review
So I wrote the patch on my laptop and then forgot and uploaded an older version from my desktop after try passed.
Attachment #473372 - Attachment is obsolete: true
Attachment #473575 - Flags: review?(ted.mielczarek)
Comment on attachment 473575 [details] [diff] [review]
Package before running PGO, v3

Yeah, I like that one better.
Attachment #473575 - Flags: review?(ted.mielczarek) → review+
Attachment #473575 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4b50cff14c74
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Blocks: 603864
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.