1495149 - Don't use signal handlers for asm.js

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Luke Wagner [:luke]]

Attachment: simplify-signal-handling

I was interested to see how much WasmSignalHandler.cpp could be simplified by removing this one particular asm.js constant-offset-folding optimizations (which wasm doesn't need b/c of non-wraparound offsets) and I realized that most simplifications are significant if we simply remove asm.js use of the signal handlers altogether: 1837 lines removed (and the majority isn't just removing the x86 decompiler).

This patch removes a bunch of complexity and probably some more in a follow-up.  Practically speaking, deployed asm.js apps have to deal with 2x worse perf due to Chrome/Safari, so the small perf hit due to removing the signal-handler optimizations on x64 shouldn't be a major problem for anyone.  It's also good to give people more of a reason to switch to wasm (in preparation for later total asm.js removal).

Comment 1

[Luke Wagner [:luke]]

Attachment: factor-simulator-fault-code

With the other use of LookupFaultingInstance() gone, it's possible to rename and repurpose this now-simulator-only function to factor out a ton of code from our 4 simulators.  Once I started doing that, I realize that the illegal-instruction fault handling could be factored out too, so I did that.

Comment 2

[Jan de Mooij [:jandem]]

That removes an impressive amount of code \o/

Comment 3

[Lars T Hansen [:lth]]

Comment on attachment 9013041 [details] [diff] [review]
simplify-signal-handling

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

Generally I'm in favor of this, though as I said on the call yesterday I am worried that some users will experience significant regressions in content that's important to them but about which we've never heard.  I am probably too pessimistic, but I just don't know.  An experiment that switches to explicit bounds checks would not be costly to do but would take a long time to yield results... :-/

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +396,5 @@
>  }
>  
> +// =============================================================================
> +// All signals/exceptions funnel down to this one trap-handling function which
> +// tests whether the pc is in a wasm module and, if so, whether there is actuall

"actually"

@@ +562,5 @@
>              return false;
>          }
>      }
>  
> +

Spurious space at start of line.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 9013409 [details] [diff] [review]
factor-simulator-fault-code

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

I approve of this cleanup :)

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9013041 [details] [diff] [review]
simplify-signal-handling

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

Nice.

::: js/src/jit/MIR.h
@@ +13559,1 @@
>      MDefinition* memoryBase() const { return getOperand(memoryBaseIndex_); }

Can you assert here hasMemoryBase()?

@@ +13615,1 @@
>      MDefinition* memoryBase() const { return getOperand(memoryBaseIndex_); }

ditto

::: js/src/jit/x86-shared/Lowering-x86-shared.cpp
@@ +296,5 @@
> +    MDefinition* boundsCheckLimit = ins->boundsCheckLimit();
> +    MOZ_ASSERT_IF(ins->needsBoundsCheck(), boundsCheckLimit->type() == MIRType::Int32);
> +
> +    // For simplicity, require a register if we're going to emit a bounds-check
> +    // branch, so that we don't have special cases for constants.

I think the reason behind this was that these bounds checks only happened on x86; maybe we could implement the constant special cases now to lower register allocation and not regress too much?

@@ +343,5 @@
> +      case Scalar::Int16: case Scalar::Uint16:
> +      case Scalar::Int32: case Scalar::Uint32:
> +      case Scalar::Float32: case Scalar::Float64:
> +        // For now, don't allow constant values. The immediate operand affects
> +        // instruction layout which affects patching.

Uh, we don't patch code anymore, so we could just remove this comment.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9013409 [details] [diff] [review]
factor-simulator-fault-code

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

Nice!

::: js/src/jit/mips32/Simulator-mips32.h
@@ +318,5 @@
> +        if (!js::wasm::MemoryAccessTraps(registerState(), (uint8_t*)addr, numBytes, &newPC)) {
> +            return false;
> +        }
> +
> +        set_pc(int32_t(newPC));

nit: set LLBit_ to false here

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +947,5 @@
> +    const wasm::ModuleSegment& segment = *codeSegment->asModule();
> +
> +    Trap trap;
> +    BytecodeOffset bytecode;
> +    if (!segment.code().lookupTrap(regs.pc, &trap, &bytecode)) {

Not sure, but don't we need to check the trap isn't OutOfBounds here?

Comment 7

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> > +    // For simplicity, require a register if we're going to emit a bounds-check
> > +    // branch, so that we don't have special cases for constants.
> 
> I think the reason behind this was that these bounds checks only happened on
> x86; maybe we could implement the constant special cases now to lower
> register allocation and not regress too much?

Good though, but as I was implementing this (which would require generalizing MacroAssembler::wasmStore() to take either a register or constant), I remember why this wasn't done in the first place: asm.js implicitly grows the minimum heap size when accessed by syntactic constants, so this case would only arise due to GVN producing a constant.  (Commented accordingly now.)

FWIW, while investigating this, I did notice that I had unnecessarily brought in the "x86 store requires useFixed(ins->value(), eax)" constraint, so I conditionalized that on #ifdef JS_CODEGEN_X86.

Comment 8

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> Not sure, but don't we need to check the trap isn't OutOfBounds here?

No, because when we fall back on a dynamic branch, we actually use masm.wasmTrap() which triggers OOB via illegal instruction.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1094983384c
Baldr: don't use signal handlers for asm.js bounds checks (r=lth,bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6344a8c25c
Baldr: factor out trap-handling code from simulators (r=lth,bbouvier)

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d1094983384c
https://hg.mozilla.org/mozilla-central/rev/8b6344a8c25c