Closed Bug 641325 Opened 14 years ago Closed 14 years ago

PGO build (RC1) doesn't merge all .pgc files (mozjs is left out)

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(blocking2.0 -)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- -

People

(Reporter: mark, Assigned: khuey)

References

Details

(Keywords: perf, regression)

Attachments

(1 file)

Patch
1021 bytes, patch
ted
: review+
sayrer
: approval2.0+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0 Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0) Gecko/20100101 Firefox/4.0 With the current build setup on FF4 RC, it seems that when building with PGO, the .pgc modules created in the profiling run aren't merged or used at all. I noticed in the build tree that they were left behind, and the build log shows: make[6]: Leaving directory `/c/mozdev/mzsrc/mozilla-2.0/build-optimized/js/src/shell' c:/mozilla-build/python/python2.6.exe /c/mozdev/mzsrc/mozilla-2.0/js/src/build/win32/pgomerge.py \ mozjs ./../../dist/ For other .pgc files, the path is */dist/firefox (where the profiling is apparently run from these days, instead of dist/bin?) and they merge correctly. This probably means that profiling data isn't used on the js engine; that is a problem as PGO will be disabled in that case for js -- I don't think you'd want that. Reproducible: Always Steps to Reproduce: 1. Set up the PGO profileserver params in .mozconfig 2. Build FF4 RC1 with make -f client.mk profiledbuild 3. Check the build log for pgomerge/link statements and check the object dir for stray .pgc files I'm also not sure if the single '.' at the start of the path is intentional, or that is should be '..' instead, like in the other paths shown in the buildlog (e.g. 'mozalloc ../../dist/firefox' and 'xul ../../dist/firefox', etc.) 'js.*' might also be influenced, but I don't think that is ever PGOd to begin with so might not be of consequence.
Keywords: perf
Version: unspecified → Trunk
After a build, I was also left with a browsercomps!1.pgc in 'dist/firefox/components' -- the build log shows a path 'browsercomps ../../../dist/firefox' for that one, instead. Possibly also the wrong path for that library.
Ted, is this a problem? Unfortunately there's no build log for the RC because the build was done manually :-/
Product: Firefox → Core
QA Contact: build.config → build-config
Odd, I'm not seeing any leftover pgc files on my trunk builds.
I'll grab a fresh tarball of RC1 without updates and attempt another build, to see if I can verify if js and mozjs really don't merge on a completely untouched tree. At the very least, I think anything that's not in /dist/(appname) but a subdirectory will always fail to merge, if I read rules.mk correctly.
I'm not really worried about browsercomps. I'd have to see a build log to confirm, but the line " mozjs ./../../dist/ " looks like it's missing $(MOZ_APP_NAME), which I guess sort of makes sense, since that's probably not defined in the JS build system. That...really sucks. I'm guessing we didn't notice this on Talos because we mostly run JIT code there. I don't know if we'd respin to fix this. It means we're not compiling the JS engine with PGO on Windows, but most of our JS perf comes from JIT, so I don't know what kind of impact it has on real-world JS perf.
Blocks: 589971
blocking2.0: --- → ?
From today's nightly: make[5]: Entering directory `/e/builds/moz2_slave/cen-w32-ntly/build/obj-firefox/js/src' link -NOLOGO -DLL -OUT:mozjs.dll -PDB:mozjs.pdb -SUBSYSTEM:WINDOWS jsanalyze.obj jsapi.obj jsarena.obj jsarray.obj jsatom.obj jsbool.obj jsclone.obj jscntxt.obj jscompartment.obj jsdate.obj jsdbgapi.obj jsdhash.obj jsdtoa.obj jsemit.obj jsexn.obj jsfriendapi.obj jsfun.obj jsgc.obj jsgcchunk.obj jsgcstats.obj jshash.obj jsinterp.obj jsinvoke.obj jsiter.obj jslock.obj jslog2.obj jsmath.obj jsnativestack.obj jsnum.obj jsobj.obj json.obj jsopcode.obj jsparse.obj jsproxy.obj jsprf.obj jsprobes.obj jspropertycache.obj jspropertytree.obj jsreflect.obj jsregexp.obj jsscan.obj jsscope.obj jsscript.obj jsstr.obj jstypedarray.obj jsutil.obj jswrapper.obj jsxdrapi.obj jsxml.obj prmjtime.obj sharkctl.obj jstracer.obj Assembler.obj Allocator.obj CodeAlloc.obj Containers.obj Fragmento.obj LIR.obj njconfig.obj RegAlloc.obj avmplus.obj Nativei386.obj jsbuiltins.obj VMPI.obj Writer.obj MethodJIT.obj StubCalls.obj Compiler.obj FrameState.obj FastArithmetic.obj FastOps.obj StubCompiler.obj MonoIC.obj PolyIC.obj ImmutableSync.obj InvokeHelpers.obj Retcon.obj TrampolineCompiler.obj checks.obj conversions.obj diy-fp.obj v8-dtoa.obj fast-dtoa.obj platform.obj utils.obj Assertions.obj ExecutableAllocatorPosix.obj ExecutableAllocatorWin.obj ExecutableAllocatorOS2.obj ExecutableAllocator.obj ARMAssembler.obj Logging.obj MacroAssemblerARM.obj MacroAssemblerX86Common.obj RegexCompiler.obj RegexJIT.obj pcre_compile.obj pcre_exec.obj pcre_tables.obj pcre_xclass.obj pcre_ucp_searchfuncs.obj CTypes.obj Library.obj jsperf.obj pm_stub.obj ctypes/libffi/.libs/libffi.lib -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -MANIFEST:NO -LIBPATH:"e:/builds/moz2_slave/cen-w32-ntly/build/obj-firefox/memory/jemalloc/crtsrc/build/intel" -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -NODEFAULTLIB:msvcprt -NODEFAULTLIB:msvcprtd -DEFAULTLIB:mozcrt19 -DEFAULTLIB:mozcpp19 -DEBUG -OPT:REF -LTCG:PGUPDATE ctypes/libffi/.libs/libffi.lib e:/builds/moz2_slave/cen-w32-ntly/build/obj-firefox/dist/lib/nspr4.lib e:/builds/moz2_slave/cen-w32-ntly/build/obj-firefox/dist/lib/plc4.lib e:/builds/moz2_slave/cen-w32-ntly/build/obj-firefox/dist/lib/plds4.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib e:/builds/moz2_slave/cen-w32-ntly/build/obj-firefox/js/src/config/nsinstall.exe -D ../../dist/sdk/lib PGOMGR : warning PG0188: No .PGC files matching 'mozjs!*.pgc' were found. mozjs.pgd(0) : warning C4961: No profile data was merged into 'mozjs.pgd', profile-guided optimizations disabled Creating library mozjs.lib and object mozjs.exp Generating code Finished generating code chmod +x mozjs.dll
Need Andreas and Brendan to weigh in, here ...
Unfortunate. Is this a recent regression?
Status: UNCONFIRMED → NEW
Ever confirmed: true
I can't see any circumstance for which I would pull the RC off the wire for this. It would be good to know though when this happened. If this has always been the case, than this falls into the roc-canvas-security-fix category: lets feel really stupid about it, and fix it in .x. There is one difference though: I would _not_ take this as a ride-along if this has been broken for more than just-the-RC-build-itself. PGO is inherently risky and needs good testing. We ran into compiler bugs before.
It looks like this regressed for beta 7.
Thanks khuey. Wow, we really really really suck. At this point I would not patch this until FF5, except if we have a lot of time for additional soaking (RC2 + 1-2 weeks of testing).
Brendan pointed out that we should probably measure the difference as an additional point of reference re: what to do.
Another valid question: assuming PGO still matters for perf, why did our test infrastructure not catch this?
I'm looking for the talos data we have been collecting on each release, where we run each suite 5 times, but currently can't locate it. If someone throws a fix at try, and a baseline, we can do some additional talos runs to assess perf impact.
A possible workaround could be to simply copy the js .pgc files to the .pgd location, and the linker will pick it up by itself, instead of trying to juggle parameters to carry over MOZ_APP_NAME to the js part? A little hack-y, I know, but it should be safe. I've manually copied it over (objdir/dist/firefox -> objdir/js/src) after the app-run this way, and building seems to be fine (box is still working on finishing up the build, but passed the js part without error)
(In reply to comment #13) > Another valid question: assuming PGO still matters for perf, why did our test > infrastructure not catch this? I posited in comment 5 that we've optimized our JIT heavily for our existing perf benchmarks, so JS PGO doesn't matter as much there. (Presumably the interpreter benefits the most from PGO.)
(In reply to comment #15) > A possible workaround could be to simply copy the js .pgc files to the .pgd > location, and the linker will pick it up by itself, instead of trying to juggle > parameters to carry over MOZ_APP_NAME to the js part? A little hack-y, I know, Well, presumably you still have to know the MOZ_APP_NAME to find where the pgc files are in the first place... I think the simplest fix here would just be to implement recursive search in the pgomerge script, which would make it work (albeit sort of in an accidental fashion, since the js Makefiles would be passing $(DIST) to the script).
The JIT calls into C heavily for string manipulation and such.
I think we should just pass MOZ_APP_NAME down to Spidermonkey. It's already got an AC_SUBST for it in MOZ_APP_NAME, and this will bite us again with something else in the future.
FWIW, you can see the regression in Dromaeo's V8 data at http://bit.ly/hC72JE, if you know that 4.0b6 was built and tested on 20100914, and 4.0b7 on 20101104. That data doesn't include 4.0rc1 (bug 641399). Doing the same with other tests we have * Dromaeo (V8) regresses by 180% (63ish to 175ms) * Dromaeo (Sunspider) regresses by about 40% * 'Sunspider' improves by about 35% (um what?) * Tp4 improves a small amount * Ts increases significantly but mostly drops back down again for b8 so probably unrelated That's all on XP (WINNT 5.1). I don't have the knowledge to comment on the real world effects of this, but the Tp4 and Ts results are encouraging.
Uh, yeah, that sounds even simpler. If you can spin a try build with a patch for that on top of the RC1 changeset, we could test the real-world perf impact of the change.
(In reply to comment #21) > Uh, yeah, that sounds even simpler. If you can spin a try build with a patch > for that on top of the RC1 changeset, we could test the real-world perf impact > of the change. On it.
Assignee: nobody → khuey
khuey's try build looks promising: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1300062967.1300088059.18642.gz&fulltext=1 d:/mozilla-build/python25/python2.5.exe /e/builds/moz2_slave/try-w32/build/js/src/build/win32/pgomerge.py \ mozjs ./../../dist/firefox Microsoft (R) Profile Guided Optimization Manager 8.00.50727.762 Copyright (C) Microsoft Corporation. All rights reserved. Merging ..\..\dist\firefox\mozjs!1.pgc <...> link -NOLOGO -DLL -OUT:mozjs.dll -PDB:mozjs.pdb -SUBSYSTEM:WINDOWS jsanalyze.obj jsapi.obj jsarena.obj jsarray.obj jsatom.obj jsbool.obj jsclone.obj jscntxt.obj jscompartment.obj jsdate.obj jsdbgapi.obj jsdhash.obj jsdtoa.obj jsemit.obj jsexn.obj jsfriendapi.obj jsfun.obj jsgc.obj jsgcchunk.obj jsgcstats.obj jshash.obj jsinterp.obj jsinvoke.obj jsiter.obj jslock.obj jslog2.obj jsmath.obj jsnativestack.obj jsnum.obj jsobj.obj json.obj jsopcode.obj jsparse.obj jsproxy.obj jsprf.obj jsprobes.obj jspropertycache.obj jspropertytree.obj jsreflect.obj jsregexp.obj jsscan.obj jsscope.obj jsscript.obj jsstr.obj jstypedarray.obj jsutil.obj jswrapper.obj jsxdrapi.obj jsxml.obj prmjtime.obj sharkctl.obj jstracer.obj Assembler.obj Allocator.obj CodeAlloc.obj Containers.obj Fragmento.obj LIR.obj njconfig.obj RegAlloc.obj avmplus.obj Nativei386.obj jsbuiltins.obj VMPI.obj Writer.obj MethodJIT.obj StubCalls.obj Compiler.obj FrameState.obj FastArithmetic.obj FastOps.obj StubCompiler.obj MonoIC.obj PolyIC.obj ImmutableSync.obj InvokeHelpers.obj Retcon.obj TrampolineCompiler.obj checks.obj conversions.obj diy-fp.obj v8-dtoa.obj fast-dtoa.obj platform.obj utils.obj Assertions.obj ExecutableAllocatorPosix.obj ExecutableAllocatorWin.obj ExecutableAllocatorOS2.obj ExecutableAllocator.obj ARMAssembler.obj Logging.obj MacroAssemblerARM.obj MacroAssemblerX86Common.obj RegexCompiler.obj RegexJIT.obj pcre_compile.obj pcre_exec.obj pcre_tables.obj pcre_xclass.obj pcre_ucp_searchfuncs.obj CTypes.obj Library.obj jsperf.obj pm_stub.obj ctypes/libffi/.libs/libffi.lib -LARGEADDRESSAWARE -NXCOMPAT -DYNAMICBASE -SAFESEH -DEBUG -DEBUGTYPE:CV -MANIFEST:NO -LIBPATH:"e:/builds/moz2_slave/try-w32/build/obj-firefox/memory/jemalloc/crtsrc/build/intel" -NODEFAULTLIB:msvcrt -NODEFAULTLIB:msvcrtd -NODEFAULTLIB:msvcprt -NODEFAULTLIB:msvcprtd -DEFAULTLIB:mozcrt19 -DEFAULTLIB:mozcpp19 -DEBUG -OPT:REF -LTCG:PGUPDATE ctypes/libffi/.libs/libffi.lib e:/builds/moz2_slave/try-w32/build/obj-firefox/dist/lib/nspr4.lib e:/builds/moz2_slave/try-w32/build/obj-firefox/dist/lib/plc4.lib e:/builds/moz2_slave/try-w32/build/obj-firefox/dist/lib/plds4.lib kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib e:/builds/moz2_slave/try-w32/build/obj-firefox/js/src/config/nsinstall.exe -D ../../dist/sdk/lib PGOMGR : warning PG0188: No .PGC files matching 'mozjs!*.pgc' were found. Creating library mozjs.lib and object mozjs.exp Generating code 682 of 13476 ( 5.06%) profiled functions will be compiled for speed 13476 of 13476 functions (100.0%) were optimized using profile data 2973600870 of 2973600870 instructions (100.0%) were optimized using profile data Finished generating code The resulting build is here: https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/khuey@kylehuey.com-f27f2aa166fa/tryserver-win32/firefox-4.0pre.en-US.win32.zip There are Talos results as well, and I spun an extra run of Dromaeo and Tp4 for each of WinXP and Win7, but I'll leave it to someone else to interpret the results.
(In reply to comment #23) > http://tbpl.mozilla.org/?tree=MozillaTry&rev=f27f2aa166fa Works like a charm :) simple and effective solution.
(In reply to comment #24) > The resulting build is here: > https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/khuey@kylehuey.com-f27f2aa166fa/tryserver-win32/firefox-4.0pre.en-US.win32.zip > > There are Talos results as well, and I spun an extra run of Dromaeo and Tp4 for > each of WinXP and Win7, but I'll leave it to someone else to interpret the > results. I'll also leave interpreting the tea leaves to the js folks. If I used the compare Talos tool right, http://bit.ly/dNiIa1 would be the comparison between the try build I ran and the build immediately before tagging the RC.
(In reply to comment #11) > Thanks khuey. Wow, we really really really suck. At this point I would not > patch this until FF5, except if we have a lot of time for additional soaking > (RC2 + 1-2 weeks of testing). Either that or we, too, like to hold back last minute changes that can goose our scores. Kyle, sdwilsh's service if offline right now. Were the gains similar to what comment 20 implies?
(In reply to comment #16) > I posited in comment 5 that we've optimized our JIT heavily for our existing > perf benchmarks, so JS PGO doesn't matter as much there. (Presumably the > interpreter benefits the most from PGO.) Which also means PGO would affect the browser chrome more than content, as the method JIT is only on by default for content at the moment.
Oh, it just came back. Topline seems to be: - small improvement in ts (1.5%) - small improvements on tp4 (4%) - small regressions in tp4_memset (2%) - 5-10% improvement in dromaeo - 10% improvement in SunSpider - 5% improvement in v8
I agree with Andreas in comment 11 - this will make a nice, basically free performance improvement for Firefox 5, but I can't see us releasing this without broader testing.
FWIW, I would feel fairly comfortable taking this patch, since we've shipped Firefox in this configuration from 3.0b4 up until 4.0b7. In all that time I personally only remember two bugs that were strictly PGO: bug 420069 (an actual bug in our code) and bug 475178 (probably a compiler bug that I worked around).
I actually agree with Ted; it's not like it's anything new, and it's been successfully used since early 3.0 versions - although I can see the "selling point" for FF5 by holding it off and using it as a bonus when then next installment is released, I also think it's safe to apply, as well as that you shouldn't keep this performance boost from the users. I'd rather see any version of FF as good as it can be.
Man, it's gonna be awesome when Firefox 5 is 10% faster on SunSpider!
blocking2.0: ? → -
let's land this on mozilla-central, since it will not be part of the mobile build.
Attachment #519125 - Flags: approval2.0+
(FWIW, the blocking decision here came from Andreas and Sayrer who were uncomfortable shipping the PGO'd version without broader testing)
Mark, I would love to see this in FF4, but its simply too risky. Turning on PGO is like touching every single line of code of the JS engine. Stuff gets moved around and can break. We need time for testing, which we do not have. We will do this in 3 month for FF5, and we have to fix our testing infrastructure to make sure this never happens again.
Andreas, fair enough :) - I guess I'll be patching this for any of my own builds, though. Mike, no need to get snarky :P - I could see more than "10% in sunspider" if you take browser chrome into account as Dave pointed out in comment 29 , and other things synthetic JS tests would have a harder time measuring. In any case, it's probably (very) important to eventually reach the "50 ms response time" goal.
Getting JaegerMonkey tested thoroughly enough to flip it on for chrome would be a far more dramatic perf boost for the browser itself than PGO, though they would certainly both help.
Can we get a test here which makes sure that the stuff we expect to end up in PGO actually does? *Pretty* please? :-)
Flags: in-testsuite?
I have no idea how you'd test this, honestly. I don't think there's any way to look at a binary and determine whether it was PGO optimized. We were passing all the right flags to the compiler and linker, we just weren't getting the profiling data to the linker. I think the right thing to focus on here is the Talos data. Did we legitimately have a 10% regression that we missed when bug 589971 landed? Or is PGO just more of a win now for whatever reason?
Is there anything in the log that we can search for to verify whether something has been PGO'ed or not?
If you look at comment 6 you'll see: mozjs.pgd(0) : warning C4961: No profile data was merged into 'mozjs.pgd', profile-guided optimizations disabled Creating library mozjs.lib and object mozjs.exp Generating code Finished generating code vs. comment 24: Creating library mozjs.lib and object mozjs.exp Generating code 682 of 13476 ( 5.06%) profiled functions will be compiled for speed 13476 of 13476 functions (100.0%) were optimized using profile data 2973600870 of 2973600870 instructions (100.0%) were optimized using profile data So I don't know how grep'able that is, but that's what it is. (Note that it's perfectly normal to have that warning for a bunch of stuff we build, because not everything gets run during the profiling phase.)
(In reply to comment #43) > If you look at comment 6 you'll see: > mozjs.pgd(0) : warning C4961: No profile data was merged into 'mozjs.pgd', > profile-guided optimizations disabled > Creating library mozjs.lib and object mozjs.exp > Generating code > Finished generating code > > vs. comment 24: > Creating library mozjs.lib and object mozjs.exp > Generating code > 682 of 13476 ( 5.06%) profiled functions will be compiled for speed > 13476 of 13476 functions (100.0%) were optimized using profile data > 2973600870 of 2973600870 instructions (100.0%) were optimized using profile > data > > So I don't know how grep'able that is, but that's what it is. (Note that it's > perfectly normal to have that warning for a bunch of stuff we build, because > not everything gets run during the profiling phase.) I think we can do something here, I've spun it off into bug 641798.
We can set this warning to be an error, we just have to be clever because we get this warning for compiled tests and it's irrelevant.
http://hg.mozilla.org/mozilla-central/rev/23455773db73 We'll deal with testing this in Bug 641798.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: