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

559961 blocks, and this basically prevents it from being used, so this should definitely block.

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.

This seems to do the trick. All green on windows try.

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.

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

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.

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.

So I wrote the patch on my laptop and then forgot and uploaded an older version from my desktop after try passed.

Comment on attachment 473575 [details] [diff] [review] Package before running PGO, v3 Yeah, I like that one better.