879891 - OdinMonkey: print warning to the console on out-of-bounds heap access

Status: RESOLVED DUPLICATE of bug 1088655

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

Attachment: mandlebrot benchmark

Attached is a benchmark that is slower in asm.js mode. I can't see a reason why that would be the case based on the source (at first i thought inlining might be relevant, so i manually inlined all the things).

OdinMonkey is about half the speed of both normal IonMonkey and Chrome, on my machine.

Comment 1

[Alon Zakai (:azakai)]

I tested a theory of luke: that while this works on doubles, perhaps the range of values they take is actually pure integers here, and the JIT ends up generating integer math but asm.js generates floating-point. However, changing all the literals into odd doubles and ensuring that the variables (except for i and j) are non-integers has no significant effect.

Comment 2

[Sean Stangl [:sstangl]]

Attachment: Shell benchmark

Comment 3

[Sean Stangl [:sstangl]]

I profiled the shell version of the benchmark: with Odin, ~2.7 seconds / ~3.9 seconds are spent entirely on a single instruction:

> movl %edx, (%r15,%rbx,1)|

Glancing at the source code, this is obviously from this line in processImage():

> image[p >> 2] = i;

Removing this line improves Odin's performance on the shell benchmark from ~2x Ion's time to slightly faster than Ion.

Comment 4

[Sean Stangl [:sstangl]]

The slowdown is almost certainly because |p| quickly exceeds the array's capacity, and we then spend most of our time going through signal handlers.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

I guess it happens only on x64 platforms, as it depends on the way out-of-bounds stores are handled. In x86, there is no significant difference between asmjs mode and no-asmjs mode.

Comment 6

[Alon Zakai (:azakai)]

Yeah, that looks like the issue. Fixing the benchmark is possible with

          while ((j|0) < 10) { // run 10 times to make benchmark longer
            p = 0;
            y = 0.0;

(add the middle line there). In that case, asm.js is a little faster than normal Firefox and Chrome.

Would it be possible to emit a console warning the first time we use a signal handler? This is basically the same idea as the asm.js compilation warning - when we are failing to execute quickly, it is nice to warn.

Comment 7

[Luke Wagner [:luke]]

That's a great idea, Alon.  Since this bug is invalid, let's morph it into adding a warning.

One annoying thing is that there isn't a super-easy way to print a warning from the out-of-bounds case.  However, we could just set a flag on the current AsmJSActivation which causes us to report a warning from C++ when we leave asm.js code.  That'd probably be good enough.  On x64, we already have the AsmJSActivation.  For x86, we could do the same, but we'd want bbouvier's patch in bug 869606 to avoid introducing serious code bloat.