785543 - Bug 778724 regressed emscripten-memops by 42%

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

Bug 778724 regressed emscripten-memops by 42%. I noticed this while verifying the fix for bug 784550, and I noticed while that bug was in fact fixed, it was then regressed by this. Unrelated I guess? This is just an unlucky benchmark ;)

The benchmark appears in bug 784550. I get 2.114 seconds when this landed,

https://hg.mozilla.org/mozilla-central/rev/bf07c6253287

versus 1.488 in the previous revision.

Comment 1

[Alon Zakai (:azakai)]

Attachment: emscripten benchmark results one revision before this regression

Running the whole emscripten benchmark suite I see some other regressions. They are small compared to the one I mentioned at the beginning (memops) though. Thought I'd paste the data though if that's helpful.

Comment 2

[Alon Zakai (:azakai)]

Attachment: emscripten benchmark results after this regression

Numbers after this regression for comparison to last comment.

Comment 3

[Brian Hackett [Laid off!]]

The problem here is that bug 778724 changed things so that uint32arrays are always treated as possibly producing doubles, which hurts JM code generation because it does not have special handling for uint32s (and IM handling for such is some ways off I think).  Fixing this is a one liner (rm 'arrayKind == TypedArray::TYPE_UINT32' from PropertyAccess in jsinfer.cpp) but doing this may incur additional unnecessary recompilation in large programs.  Alon, does this affect more substantial programs?

Comment 4

[Alon Zakai (:azakai)]

It's harder to benchmark entire programs, but the benchmark suite does contain real-world code and not tiny kernels. For example dlmalloc which is included in many large programs regressed by 33%. So I would guess yes.

On the other hand, I just tested bullet and box2d from

https://github.com/kripken/misc-js-benchmarks

and I do *not* see a regression there, performance is unchanged. And they are much more serious than the benchmark suite. But - they are just 2 tests.

So I don't have a simple answer here I am afraid.

Comment 5

[Alon Zakai (:azakai)]

Your description of the problem scares me, though - it sounds like any code that does a lot of Uint32Array accesses will be slower? That should be quite a lot of code. Perhaps bullet and box2d avoid the problem since they are mainly float ops.

In fact this seems to make sense since dlmalloc and memops, the worst regressions in the benchmark suite, have no floats but just int operations and memory accesses.

So if code that does a lot of int operations with heap accesses is at risk, this is concerning. Perhaps I did not understand the explanation of the issue though.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

Fair enough, thanks for looking.  The real long term fix here is to have a compiler that deals well with native uint32s, until then I think we need to just eat any recompilations we get from not knowing which uint32array reads are int32 vs. uint32.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: patch

Oops, wrong patch.

Comment 8

[Alon Zakai (:azakai)]

I know ~0 about the implementation here, but looking at the patch, it seems it no longer marks Uint32 arrays as potentially producing doubles. And I guess if/when doubles actually appear they would trigger recompilation? This seems like the right approach since values big enough to be doubles are fairly rare in compiled code, so the recompiles won't be so bad, and normal code without doubles will be fast like before. Did I understand that right?

Btw, v8 just landed good support for uint32s,

http://code.google.com/p/v8/issues/detail?id=2097

so would be nice to eventually have that in IonMonkey.

Comment 9

[Brian Hackett [Laid off!]]

(In reply to Alon Zakai (:azakai) from comment #8)
> I know ~0 about the implementation here, but looking at the patch, it seems
> it no longer marks Uint32 arrays as potentially producing doubles. And I
> guess if/when doubles actually appear they would trigger recompilation? This
> seems like the right approach since values big enough to be doubles are
> fairly rare in compiled code, so the recompiles won't be so bad, and normal
> code without doubles will be fast like before. Did I understand that right?

Yes, this is right.

Comment 10

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a6fa8dd582b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I don't know if this is coincidence or not because it hasn't happened since, but this push had a Linux64 debug jit-test failure.

https://tbpl.mozilla.org/php/getParsedLog.php?id=14701711&tree=Mozilla-Inbound
TEST-UNEXPECTED-FAIL | jit_test.py -a -m -d -n    | /builds/slave/m-in-lnx64-dbg/build/js/src/jit-test/tests/basic/bug688939.js: Assertion failure: first - 1 <= last, at ../../../js/src/gc/Heap.h:340

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a6fa8dd582b

Comment 13

[Alon Zakai (:azakai)]

(In reply to Brian Hackett (:bhackett) from comment #9)
> (In reply to Alon Zakai (:azakai) from comment #8)
> > I know ~0 about the implementation here, but looking at the patch, it seems
> > it no longer marks Uint32 arrays as potentially producing doubles. And I
> > guess if/when doubles actually appear they would trigger recompilation? This
> > seems like the right approach since values big enough to be doubles are
> > fairly rare in compiled code, so the recompiles won't be so bad, and normal
> > code without doubles will be fast like before. Did I understand that right?
> 
> Yes, this is right.

And is the reason we were considering changing this to the less-performant but fewer-recompilation version because recompilations take memory?

Comment 14

[Brian Hackett [Laid off!]]

Bug 778724 wasn't meant to be less performant here, this was a simple oversight.

Comment 15

[Alon Zakai (:azakai)]

Oh ok, thanks. Just curious about the sequence of events here.