Closed Bug 883789 Opened 9 years ago Closed 9 years ago

Regression on box2d after bug 882925

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: h4writer, Unassigned)

Details

Attachments

(1 file)

Bug 882925 fixed a typo in baseline. This caused some strange behaviour. Awfy shows regressions for this revision (5.7%). Upon trying home (32bit), I get a speed-up. Also on revision that shouldn't impact the generated code we get our regression back on 32bit only. So something strange is going on.

djvj wrote:
> This is likely an issue where we wrongly specialize ion code to integers, and then bail when >doubles show up.  There may be long runs of integers that span past the 1000 use count limit before compiling to Ion.
>The solution might be to remember the overflow bit in the fallback stub instead of just scanning the optimized stubs.  That way a GC can't wipe out that info.

That would possible explain speed-ups/regressions. Where for the regression the timings of GC are this way that we don't see the doubles in baseline, but only the integers. And a little shift/longer compilation/... causes us to see the double and correctly identify an overflow, resulting in better IM that doesn't need to bail to handle the overflow.
Speculative atm...
Is this also the cause of the ~14% regression on x64? It's in the right range, but it looks like AWFY didn't run x64 for a while, so the range is pretty big:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=18c1fd169792&tochange=e6918653f914
I found another issue.  In |BaselineICInspector::expectedResultType|, we count BinaryArith_DoubleWithInt32 as a double-yielding stub, but that's not the case.  Those stubs are for bit operations AND,OR,and XOR, which always yield int32 results.

Implementing independent overflow bit tracking in baseline (and changing Ion to use it) and fixing this expectedResultType seems to bring us back on Box2D.
Attached patch Patch.Splinter Review
Attachment #764297 - Flags: review?(hv1989)
Comment on attachment 764297 [details] [diff] [review]
Patch.

Review of attachment 764297 [details] [diff] [review]:
-----------------------------------------------------------------

In the end I didn't check the result on the awfy itself. I think it will be easier to just land and see if it indeed fixes the issue there.
Even if it doesn't fix the regression there, we want this in the tree.
Testing on my own computer showed an improvement too, nice!

::: js/src/ion/BaselineIC.cpp
@@ +2468,5 @@
>          return false;
>      }
>  
> +    if (ret.isDouble())
> +        stub->setSawOverflow();

So this means we also set this flag when we have Double X Double arith? Does that makes sense?

::: js/src/ion/BaselineInspector.cpp
@@ +164,5 @@
>              return MIRType_Double;
>          return MIRType_Int32;
>        case ICStub::BinaryArith_BooleanWithInt32:
>        case ICStub::UnaryArith_Int32:
> +      case ICStub::BinaryArith_DoubleWithInt32:

Nice find!
Attachment #764297 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #4)
> So this means we also set this flag when we have Double X Double arith? Does
> that makes sense?

Good point.  The word 'overflow' is a bit misleading here.  Maybe it should be called "sawDoubleResult" instead.  The bit is used to set the MIRType of certain MInstructions to MIRType_Double.

I'm going to change it from "sawOverflow" to "sawDoubleResult".
https://hg.mozilla.org/mozilla-central/rev/6d4a482b6aa7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.