Omnijar before profiling part of PGO

RESOLVED FIXED in mozilla2.0b7

Status

defect
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: taras.mozilla, Assigned: mwu)

Tracking

unspecified
mozilla2.0b7
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [ts])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

9 years ago
Currently omnijaring happens at packaging which means optimized jar ordering from bug 559961 isn't getting triggered.
Reporter

Updated

9 years ago
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.

Comment 3

9 years ago
Yeah, we should profile packaged builds: the startup perf is probably different-enough that we might not PGO the JAR-loading bits correctly.

Updated

9 years ago
blocking2.0: ? → final+
Assignee

Comment 4

9 years ago
Posted patch Package before running PGO (obsolete) — Splinter Review
This seems to do the trick. All green on windows try.
Attachment #470490 - Flags: review?(benjamin)
Assignee

Updated

9 years ago
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-
Assignee

Comment 6

9 years ago
(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.
Assignee

Comment 8

9 years ago
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+
Assignee

Comment 10

9 years ago
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+
Assignee

Comment 12

9 years ago
Attachment #473575 - Attachment is obsolete: true
Assignee

Updated

9 years ago
Keywords: checkin-needed

Comment 13

9 years ago
http://hg.mozilla.org/mozilla-central/rev/4b50cff14c74
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Blocks: 603864

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.