Make IONFLAGS work in optimized builds

RESOLVED FIXED in Firefox 45

Status

()

Product:
Core
Component:
JavaScript Engine: JIT
Type:
defect
Status:
RESOLVED FIXED
Opened:
5 years ago
Updated:
4 years ago

People

(Reporter: h4writer, Assigned: lth, Mentored)

Tracking

Version:
unspecified
Target:
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(7 attachments, 9 obsolete attachments)

WIP
11.74 KB, patch
Details | Diff | Splinter Review
[WIP] JitSpew as Macro
5.04 KB, patch
Details | Diff | Splinter Review
benchmarks
10.98 KB, text/plain
Details
[WIP] Ionflags for optimize builds Patch1
31.25 KB, patch
Details | Diff | Splinter Review
bug1094150-jitspew.patch
17.16 KB, patch
nbp
: review+
Details | Diff | Splinter Review
scandebug.py
855 bytes, text/x-python-script
Details
bug1094150-more-channels.patch
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
Mentor: hv1989
Hey,
I'm ready to take this up if this bug is still free.
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
Thanks Till. I will get right to it!
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 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+
Posted patch WIP — — Splinter Review 
Hannes, could you fire off a custom AWFY run to see what the perf regressions
are?
Attachment #8536914 - Flags: feedback?(hv1989)
(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.
Addressed the above comments.
Attachment #8535465 - Attachment is obsolete: true
Attachment #8537618 - Flags: feedback?(nicolas.b.pierron)
Attachment #8537618 - Flags: feedback?(nicolas.b.pierron)
Posted file benchmarks (obsolete) — 
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)
(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" ?
Attachment #8537619 - Flags: feedback?(nicolas.b.pierron)
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
@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 :)
Posted file benchmarks (obsolete) — 
Updated benchmarks with more information.
Attachment #8537619 - Attachment is obsolete: true
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.
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.
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?
(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?
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)
Attachment #8537618 - Attachment is obsolete: true
Added JitSpew as a Macro.
Posted file benchmarks (obsolete) — 
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)
Attachment #8542851 - Attachment is obsolete: true
Posted file benchmarks (obsolete) —
Attachment #8542857 - Attachment is obsolete: true
Attachment #8542857 - Flags: feedback?(hv1989)
Attachment #8544764 - Flags: feedback?(hv1989)
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.
Attachment #8544764 - Flags: feedback?(hv1989)
Posted file benchmarks —
Attachment #8544764 - Attachment is obsolete: true
Attachment #8544762 - Attachment is obsolete: true
Assignee: nobody → eternalsushant
Is there any more progress on this. It would still be nice to have.
Grumble.  I had to reinvent this today; I'll attach a patch of what I had.
Posted patch bug1094150-jitspew.patch (obsolete) — — Splinter Review 
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 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.
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)
Assignee: eternalsushant → lhansen
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+
(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.
Posted file scandebug.py — 
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.)
Flags: needinfo?(lhansen)
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.
(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.
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 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+
https://hg.mozilla.org/mozilla-central/rev/406ec8b601f0
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.