Closed
Bug 1094150
Opened 8 years ago
Closed 7 years ago
Make IONFLAGS work in optimized builds
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: h4writer, Assigned: lth, Mentored)
Details
Attachments
(7 files, 9 obsolete files)
11.74 KB,
patch
|
Details | Diff | Splinter Review | |
5.04 KB,
patch
|
Details | Diff | Splinter Review | |
10.98 KB,
text/plain
|
Details | |
31.25 KB,
patch
|
Details | Diff | Splinter Review | |
17.16 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
855 bytes,
text/x-python-script
|
Details | |
28.82 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
The IONFLAGS environment flag is often used when searching for performance issues and fixing them. It enables jit engine developers see the different steps in the jit engine, the resulting code, bailouts ... So very important. Currently this only works in a DEBUG build. We want to enable this on optimized builds also, since the logs have more meaning. So we can have a closer look at our generated code and decisions made in an optimized build (instead of currently only debug builds). This bug requests the following changes: 1) Currently we toggle everything based if DEBUG is defined. Part 1 is to introduce JS_JITSPEWER. Which is set in "mozilla-inbound/configure.in" when DEBUG is enabled (browser) and is always set in mozilla-inbound/configure.in (shell). 2) Adjust all files to test for JS_JITSPEWER instead of DEBUG to enable jitspewer. E.g: http://dxr.mozilla.org/mozilla-central/source/js/src/jit/JitSpewer.cpp?from=JitSpewer.cpp&case=true#7 http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp?from=IonBuilder.cpp#177 ^ note: in last case we still need PCToLineNumber to be DEBUG only ... 3) Test that if everything we don't spew anything we have no regressions on octane/ss/kraken. If we do have regressions: search wat caused it and solve it or still only enable that functionality for DEBUG only
Reporter | ||
Updated•8 years ago
|
Mentor: hv1989
Comment 1•8 years ago
|
||
Hey, I'm ready to take this up if this bug is still free.
Comment 2•8 years ago
|
||
Hey Sushant, that's great to hear! Feel free to just dive in, nobody's yet working on this.
Summary: Make IONFLAGS work in optimized shell builds → Make IONFLAGS work in optimized builds
Comment 3•8 years ago
|
||
Thanks Till. I will get right to it!
Comment 4•8 years ago
|
||
WIP Patch. - Added JS_JITSPEWER in js/src/configure.in - Changed #ifdef DEBUG to use JS_JITSPEWER instead in places where JitSpew was called. I did leave out a few places though as they called PCToLineNumber or had some other dependency. I've left comments in these places to address the same later. Please comment on the same and also about what's left to be done.
Attachment #8535465 -
Flags: feedback?(nicolas.b.pierron)
Comment 5•8 years ago
|
||
Comment on attachment 8535465 [details] [diff] [review] [WIP] Ionflags for optimize builds Review of attachment 8535465 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +3910,5 @@ > unsigned lineNumber = 0, columnNumber = 0; > if (current->mir()->info().script()) { > filename = current->mir()->info().script()->filename(); > if (current->mir()->pc()) > lineNumber = PCToLineNumber(current->mir()->info().script(), current->mir()->pc(), Use #ifdef DEBUG around these 2 lines, and change the above by #ifdef JS_JITSPEWER. Having incomplete information is better than having no information. ::: js/src/jit/Safepoints.cpp @@ +279,5 @@ > SafepointWriter::writeNunboxParts(LSafepoint *safepoint) > { > LSafepoint::NunboxList &entries = safepoint->nunboxParts(); > > # ifdef DEBUG # if defined(DEBUG) && defined(JS_JITSPEWER)
Attachment #8535465 -
Flags: feedback?(nicolas.b.pierron) → feedback+
Comment 6•8 years ago
|
||
Hannes, could you fire off a custom AWFY run to see what the perf regressions are?
Attachment #8536914 -
Flags: feedback?(hv1989)
Comment 7•8 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #6) > Created attachment 8536914 [details] [diff] [review] > WIP > > Hannes, could you fire off a custom AWFY run to see what the perf regressions > are? The patch probably doesn't compile for ARM and MIPS -- that's fine for now, I'm just interested to see the x86/x64 perf regressions.
Comment 8•8 years ago
|
||
Addressed the above comments.
Attachment #8535465 -
Attachment is obsolete: true
Attachment #8537618 -
Flags: feedback?(nicolas.b.pierron)
Updated•8 years ago
|
Attachment #8537618 -
Flags: feedback?(nicolas.b.pierron)
Comment 9•8 years ago
|
||
So I ran a few benchmarks on my PC (Ubuntu 12.04) and here are the results. Do you think the patch is ready for r? ? (I'm not sure as to what else has to be done)
Attachment #8537619 -
Flags: feedback?(nicolas.b.pierron)
Comment 10•8 years ago
|
||
(In reply to Sushant Dinesh from comment #9) > Created attachment 8537619 [details] > benchmarks > > So I ran a few benchmarks on my PC (Ubuntu 12.04) and here are the results. > Do you think the patch is ready for r? ? (I'm not sure as to what else has > to be done) What are the totals for sunspider and kraken? What are the configure flags for "before" and "After"? How many times did each benchmarks ran? What is the "uncertainty" ?
Updated•8 years ago
|
Attachment #8537619 -
Flags: feedback?(nicolas.b.pierron)
Comment 11•8 years ago
|
||
If performance is a problem, I recommend making JitSpew a macro, like what I did in the WIP patch: +extern uint32_t LoggingBits; +#define JitSpew(...) \ + JS_BEGIN_MACRO \ + if (LoggingBits != 0) \ + JitSpewSlow(__VA_ARGS__); \ + JS_END_MACRO
Comment 12•8 years ago
|
||
@shu, Hey! I'm actually not sure how to go about this. I'm just following what the bug description says as I had not discussed this with h4writer. We can discuss this on IRC and try it out too :)
Comment 13•8 years ago
|
||
Updated benchmarks with more information.
Attachment #8537619 -
Attachment is obsolete: true
Comment 14•8 years ago
|
||
Comment on attachment 8539039 [details]
benchmarks
@nbp, I updated the benchmarks with the information you requested. Please take a look and tell me what you think.
Comment 15•8 years ago
|
||
The benchmarks results are quite noisy, usually we hundreds runs, you should have a smaller variance. Usually before running the benchmarks I do: echo performance | sudo tee /sys/bus/cpu/devices/cpu*/cpufreq/scaling_governor pkill -s 19 firefox # Send a SIGSTOP to process which might be using the CPU and after: pkill -s 18 firefox # Send a SIGCONT to resume executions. Otherwise this looks good except for sunspider-crypto-aes, which might still be a noise issue. I like shu solution about adding the macro to wrap the function call, at least this would remove the argument computation, but this will add warning about unused variable in the cases where we compute the spewed content before the call.
Reporter | ||
Comment 16•8 years ago
|
||
Comment on attachment 8537618 [details] [diff] [review] [WIP] Ionflags for optimize builds Review of attachment 8537618 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/CodeGenerator.cpp @@ +3914,3 @@ > if (current->mir()->pc()) > lineNumber = PCToLineNumber(current->mir()->info().script(), current->mir()->pc(), > &columnNumber); I think this can be done when DEBUG is not defined. Can you confirm (looking in the code and see it is defined and should work? @@ +3915,5 @@ > lineNumber = PCToLineNumber(current->mir()->info().script(), current->mir()->pc(), > &columnNumber); > } else { > lineNumber = current->mir()->lineno(); > columnNumber = current->mir()->columnIndex(); Can you update to the correct ifdefs? I think lineno/columnIndex has other IFDEFs than DEBUG. So can you update this? ::: js/src/jit/Safepoints.cpp @@ +279,5 @@ > SafepointWriter::writeNunboxParts(LSafepoint *safepoint) > { > LSafepoint::NunboxList &entries = safepoint->nunboxParts(); > > +# if defined(DEBUG) && defined(JS_JITSPEWER) What is the reason we can't remove the DEBUG test here?
Reporter | ||
Comment 17•8 years ago
|
||
(In reply to Sushant Dinesh from comment #13) > Created attachment 8539039 [details] > benchmarks > > Updated benchmarks with more information. Sorry about the delay. Plowing through my backlog. It seems like there is a regression according to those runs. Can you increase the number of runs running the browser (./sunspider --runs=X) and remove possible noise (e.g. close all other applications esp. the browser when running) Secondly, can you look at the patch of shu for how he adjusted JitSpew(...) with a macro? Can you incorporate that in your patch and again measure the difference with a plain run?
Reporter | ||
Comment 18•8 years ago
|
||
Comment on attachment 8536914 [details] [diff] [review] WIP Review of attachment 8536914 [details] [diff] [review]: ----------------------------------------------------------------- I cannot start custom runs of AWFY just yet. That's on my wishlist for new year ;).
Attachment #8536914 -
Flags: feedback?(hv1989)
Comment 19•8 years ago
|
||
Attachment #8537618 -
Attachment is obsolete: true
Comment 20•8 years ago
|
||
Added JitSpew as a Macro.
Comment 21•8 years ago
|
||
Updated benchmarks. Ran the tests for 100 runs. Here are the results. All js shells were build with: CC="gcc -m32" CXX="g++ -m32" AR=ar ../configure --disable-debug --enable-optimize --target=i686-pc-linux-gnu --enable-build-nspr Looking at it, it seems like there is a regression. If so, what do we do next? ;)
Attachment #8539039 -
Attachment is obsolete: true
Attachment #8542857 -
Flags: feedback?(hv1989)
Comment 22•8 years ago
|
||
Attachment #8542851 -
Attachment is obsolete: true
Comment 23•8 years ago
|
||
Attachment #8542857 -
Attachment is obsolete: true
Attachment #8542857 -
Flags: feedback?(hv1989)
Attachment #8544764 -
Flags: feedback?(hv1989)
Reporter | ||
Comment 24•8 years ago
|
||
Comment on attachment 8544762 [details] [diff] [review] [WIP] Ionflags for optimize builds Review of attachment 8544762 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/JitSpewer.cpp @@ +91,5 @@ > void > jit::IonSpewPass(const char *pass) > { > if (GetJitContext()->runtime->onMainThread()) > ionspewer.spewPass(pass); You should have added the "JitSpewEnabled" for all functions with "GetJitContext". There are 3 extra functions where this test needs to get added IIRC.
Reporter | ||
Updated•8 years ago
|
Attachment #8544764 -
Flags: feedback?(hv1989)
Comment 25•8 years ago
|
||
Attachment #8544764 -
Attachment is obsolete: true
Comment 26•8 years ago
|
||
Attachment #8544762 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → eternalsushant
Comment 27•8 years ago
|
||
Is there any more progress on this. It would still be nice to have.
Assignee | ||
Comment 28•7 years ago
|
||
Grumble. I had to reinvent this today; I'll attach a patch of what I had.
Assignee | ||
Comment 29•7 years ago
|
||
I created this to allow codegen spew in release builds (without all the debug instructions fouling up the code), it's probably not a complete fix but it's not the first time I write this code so it'd be nice to land it after a little cleanup. The only cleanup I know is needed for sure is that there's no build system support here yet, there's a hacked moz.build that always enables JITSPEW but it should do that unconditionally in debug builds and with a suitable configuration flag in release builds. This probably isn't a complete solution for other aspects than codegen, but if we land this one as a base patch we can add other kinds of spew piecemeal.
Comment 30•7 years ago
|
||
Comment on attachment 8680002 [details] [diff] [review] bug1094150-jitspew.patch Review of attachment 8680002 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BacktrackingAllocator.cpp @@ +1416,2 @@ > JitSpew(JitSpew_RegAlloc, " %s collides with %s [weight %lu]", > r.reg.name(), existing->toString(), computeSpillWeight(existing)); nit: You might as well wrap the JitSpew call in it. ::: js/src/jit/BaselineBailouts.cpp @@ +1425,1 @@ > MOZ_ASSERT(prevFrameType == JitFrame_IonJS || I think we also want to check this assertion in DEBUG && !JITSPEW builds.
Assignee | ||
Comment 31•7 years ago
|
||
Nits fixed (thanks!), configure system fixed (to the best of my limited ability), and I added support for IONFLAGS=scripts, which seems useful. As for the other flags we'll have to go over the code for each of them, but that can be a followup perhaps.
Attachment #8680002 -
Attachment is obsolete: true
Attachment #8680072 -
Flags: review?(nicolas.b.pierron)
Updated•7 years ago
|
Assignee: eternalsushant → lhansen
Comment 32•7 years ago
|
||
Comment on attachment 8680072 [details] [diff] [review] bug1094150-jitspew.patch Review of attachment 8680072 [details] [diff] [review]: ----------------------------------------------------------------- This sounds good to me. ::: js/src/jit/BacktrackingAllocator.cpp @@ +1418,4 @@ > } else { > JitSpew(JitSpew_RegAlloc, " %s collides with the following", r.reg.name()); > for (size_t i = 0; i < aliasedConflicting.length(); i++) { > +#ifdef JS_JITSPEW nit: you might as well wrap the condition if (JitSpewEnabled(…))
Attachment #8680072 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 33•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fa48045cc46
Assignee | ||
Comment 34•7 years ago
|
||
(In reply to Lars T Hansen [:lth] from comment #33) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fa48045cc46 There are many failures there. All appear to be the same infra issue: something about hg log being abused. There was a brief discussion about this on IRC last night. :sfink thought it might have something to do with trying to support git. I'm going to ignore the problem, though I fear that the problem generally may result in fewer things being tested on try.
Assignee | ||
Comment 35•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a8ba0c340f9ffe3858b8733b9182ccc5ff86bf Bug 1094150 - make jitspew available in release builds. r=nbp
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Comment 36•7 years ago
|
||
A simple script to look for instances of "JitSpew_" inside DEBUG blocks, which we can use to find other locations that need fixing. (Additionally I saw an instance where JitSpew_ is being used on the RHS of a #define, needs to be handled separately.)
Comment 37•7 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4b0ee507c8
Comment 38•7 years ago
|
||
backed out for problems like https://treeherder.mozilla.org/logviewer.html#?job_id=16542765&repo=mozilla-inbound
Flags: needinfo?(lhansen)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(lhansen)
Assignee | ||
Comment 39•7 years ago
|
||
The failures are very curious since there should be no functional difference in this patch from what came before in either release or debug builds. The only change should come about in manually configured release builds. The only probable explanation is that JS_JITSPEW is not properly enabled in debug builds on the buildbot (ie a bug in my configure.in / moz.build changes) and that this breaks some invariants, strange as that may sound. It's not a great explanation since the failures are not exactly predictable, yet repeatable, but it's all I have.
Assignee | ||
Comment 40•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b6f9557c40
Assignee | ||
Comment 41•7 years ago
|
||
(In reply to Lars T Hansen [:lth] from comment #40) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b6f9557c40 Vastly improved with a small fix for the config files.
Assignee | ||
Comment 42•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2638fcfb3217dbe24b1052fa2f44335d44ef5de5 Bug 1094150 - make jitspew available in release builds. r=nbp
Assignee | ||
Comment 43•7 years ago
|
||
This patch changes DEBUG to JS_JITSPEW to cover the rest of the channels, so far as I've found them. (Basically by grepping for JitSpew_ inside DEBUG sections of the source.) In one or two places I've dependend on the fact that DEBUG implies JS_JITSPEW, so spew code inside bona fide DEBUG-only code is not available in non-DEBUG builds. The case in JitFrames.cpp is most prominent, look for verifyReturnAddressUsingNativeToBytecodeMap in the source (not in the patch).
Attachment #8681614 -
Flags: review?(nicolas.b.pierron)
Comment 44•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2638fcfb3217
Comment 45•7 years ago
|
||
Comment on attachment 8681614 [details] [diff] [review] bug1094150-more-channels.patch Review of attachment 8681614 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h @@ +456,1 @@ > if (numPoolEntries) { follow-up nit: Add JitSpewEnabled(JitSpew_Pools) in this condition.
Attachment #8681614 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 46•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89df5c6582a4
Assignee | ||
Comment 47•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/406ec8b601f0cc5795c0c10b851ebc17b856a89b Bug 1094150 - more JitSpew channels. r=nbp
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 48•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/406ec8b601f0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 49•7 years ago
|
||
bugherder |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2638fcfb3217 https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/406ec8b601f0
status-b2g-v2.5:
--- → fixed
Comment 50•7 years ago
|
||
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•