1094150 - Make IONFLAGS work in optimized builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

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

Comment 1

[Sushant Dinesh]

Hey,
I'm ready to take this up if this bug is still free.

Comment 2

[Till Schneidereit [:till]]

Hey Sushant, that's great to hear! Feel free to just dive in, nobody's yet working on this.

Comment 3

[Sushant Dinesh]

Thanks Till. I will get right to it!

Comment 4

[Sushant Dinesh]

Attachment: [WIP] Ionflags for optimize builds

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.

Comment 5

[Nicolas B. Pierron [:nbp]]

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)

Comment 6

[Shu-yu Guo [:shu]]

Attachment: WIP

Hannes, could you fire off a custom AWFY run to see what the perf regressions
are?

Comment 7

[Shu-yu Guo [:shu]]

(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

[Sushant Dinesh]

Attachment: [WIP] Ionflags for optimize builds

Addressed the above comments.

Comment 9

[Sushant Dinesh]

Attachment: 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)

Comment 10

[Nicolas B. Pierron [:nbp]]

(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" ?

Comment 11

[Shu-yu Guo [:shu]]

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

[Sushant Dinesh]

@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

[Sushant Dinesh]

Attachment: benchmarks

Updated benchmarks with more information.

Comment 14

[Sushant Dinesh]

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

[Nicolas B. Pierron [:nbp]]

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 16

[Hannes Verschore [:h4writer]]

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?

Comment 17

[Hannes Verschore [:h4writer]]

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

[Hannes Verschore [:h4writer]]

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 ;).

Comment 19

[Sushant Dinesh]

Attachment: [WIP] Ionflags for optimize builds

Comment 20

[Sushant Dinesh]

Attachment: [WIP] JitSpew as Macro

Added JitSpew as a Macro.

Comment 21

[Sushant Dinesh]

Attachment: benchmarks

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? ;)

Comment 22

[Sushant Dinesh]

Attachment: [WIP] Ionflags for optimize builds

Comment 23

[Sushant Dinesh]

Attachment: benchmarks

Comment 24

[Hannes Verschore [:h4writer]]

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.

Comment 25

[Sushant Dinesh]

Attachment: benchmarks

Comment 26

[Sushant Dinesh]

Attachment: [WIP] Ionflags for optimize builds Patch1

Comment 27

[Michael Bebenita [:mbx]]

Is there any more progress on this. It would still be nice to have.

Comment 28

[Lars T Hansen [:lth]]

Grumble.  I had to reinvent this today; I'll attach a patch of what I had.

Comment 29

[Lars T Hansen [:lth]]

Attachment: bug1094150-jitspew.patch

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

[Nicolas B. Pierron [:nbp]]

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.

Comment 31

[Lars T Hansen [:lth]]

Attachment: bug1094150-jitspew.patch

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.

Comment 32

[Nicolas B. Pierron [:nbp]]

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(…))

Comment 33

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fa48045cc46

Comment 34

[Lars T Hansen [:lth]]

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

Comment 35

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a8ba0c340f9ffe3858b8733b9182ccc5ff86bf
Bug 1094150 - make jitspew available in release builds.  r=nbp

Comment 36

[Lars T Hansen [:lth]]

Attachment: 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.)

Comment 37

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4b0ee507c8

Comment 38

[Carsten Book [:Tomcat]]

backed out for problems like https://treeherder.mozilla.org/logviewer.html#?job_id=16542765&repo=mozilla-inbound

Comment 39

[Lars T Hansen [:lth]]

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.

Comment 40

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b6f9557c40

Comment 41

[Lars T Hansen [:lth]]

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

Comment 42

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2638fcfb3217dbe24b1052fa2f44335d44ef5de5
Bug 1094150 - make jitspew available in release builds.  r=nbp

Comment 43

[Lars T Hansen [:lth]]

Attachment: bug1094150-more-channels.patch

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

Comment 44

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2638fcfb3217

Comment 45

[Nicolas B. Pierron [:nbp]]

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.

Comment 46

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89df5c6582a4

Comment 47

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/406ec8b601f0cc5795c0c10b851ebc17b856a89b
Bug 1094150 - more JitSpew channels.  r=nbp

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/406ec8b601f0

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2638fcfb3217
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/406ec8b601f0

Comment 50

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion