1271010 - Baldr: add real heterogeneous function table

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Currently, the wasm heterogeneous table is simulated by N asm.js-style homogeneous tables which has pretty significant overhead in large apps (~40MiB).  This bug will add actual heterogeneous tables that do dynamic signature checks; the overhead should be minimal (given the indirect call), so it seems like the right tradeoff.

Comment 1

[Luke Wagner [:luke]]

Attachment: simplify-regs

The 'Volatile' requirement for registers clobbered during the profiling prologue came from back when there were these native profiling thunks (where the caller of native was assuming non-volatile regs were preserved).  That's no longer the case so we can use a much simpler register scheme (which will be used in the later patch).

Comment 2

[Luke Wagner [:luke]]

Attachment: rename-callargs

Random cleanup while I was in the area: 'Call' is too broad of a name for this struct that encapsulates call args.

Comment 3

[Luke Wagner [:luke]]

Attachment: hetero-tables (WIP)

Here's a half-done patch, which goes so far as to create 1 heterogeneous table and pass the signature in a non-arg register.  The missing bit is the thunk that catches signature mismatches, but this is enough to run the wasm demo (or any code that doesn't have signature mismatches) and confirm the size drop of 'class(WasmModuleObject)/objects' from 72.64 MB to 31.78MB.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8749901 [details] [diff] [review]
rename-callargs

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

Nice!

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: remove-asmjsinternalcallee.patch

We don't need AsmJSInternalCallee anymore, as SymbolicAddress is now an enum class and not just a simple enum (which the compiler could mix with an uint32_t, thus calling MacroAssembler::call(AsmJSImmKind) instead of MacroAssembler::call(uint32_t)).

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8749900 [details] [diff] [review]
simplify-regs

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

Great! Less platform dependent code is always better.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8750798 [details] [diff] [review]
remove-asmjsinternalcallee.patch

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

Yes, good point.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8b1991d802

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a8b1991d802

Comment 10

[Luke Wagner [:luke]]

Attachment: factor-code-range

Simple patch preparing for next patch.  CodeRange will need to get a bit bigger.  It's only a few hundred KB, though, and we can reoptimize CodeRange later.

Comment 11

[Luke Wagner [:luke]]

Attachment: factor-patchable-jump

This patch moves the nop-to-jump patching into the MacroAssembler so it can be reused by the next patch (and it's cleaner anyhow).

Comment 12

[Luke Wagner [:luke]]

Attachment: hetero-tables

The strategy I found is:
 - a "table entry" (sibling to "profiling entry" and "non-profiling entry") is added to every function (we can optimize later to only generate when needed)
 - the table entry takes an extra argument in a non-argument register, compares it against the callee's signature and jumps to a throw on mismatch
 - since the table entry is right before the non-profiling entry, it just falls through if no mismatch
 - when profiling is enabled, a nop is patched so the table entry jumps to the profiling entry instead
 - wasm tables exclusively contain pointers to the table-entry

Overall, pretty simple.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8756702 [details] [diff] [review]
factor-patchable-jump

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

Muuuch cleaner. Thanks.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +4845,5 @@
> +void
> +MacroAssembler::patchNopToJump7(uint8_t* jump, uint8_t* target)
> +{
> +    MOZ_ASSERT(reinterpret_cast<Instruction*>(jump)->is<InstNOP>());
> +    new (jump) InstBImm(BOffImm(target - jump), Assembler::Always);

Could we release_assert that target >= jump?

::: js/src/jit/x86-shared/BaseAssembler-x86-shared.h
@@ +76,5 @@
> +    {
> +        // Note: the offset is relative to the address of the instruction after
> +        // the jump which is two bytes.
> +        ptrdiff_t rel8 = target - jump - 2;
> +        MOZ_RELEASE_ASSERT(rel8 >= INT8_MIN && rel8 <= INT8_MAX);

Note to self: this also asserts if `target - jump` underflows, or `target - jump - 2` underflows.

Comment 14

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> > +    new (jump) InstBImm(BOffImm(target - jump), Assembler::Always);
> 
> Could we release_assert that target >= jump?

Actually, the jumps can go backwards too (in the next patch).

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #14)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #13)
> > > +    new (jump) InstBImm(BOffImm(target - jump), Assembler::Always);
> > 
> > Could we release_assert that target >= jump?
> 
> Actually, the jumps can go backwards too (in the next patch).

Right. Could we have assertions like on x86/x64 that the immediate fits the target range then?

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8756705 [details] [diff] [review]
hetero-tables

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

It really is simple. Thank you for the patch!

::: js/src/asmjs/WasmFrameIterator.cpp
@@ +769,5 @@
>      MacroAssembler::repatchThunk(module.code(), callThunk.offset, calleeOffset);
>  }
>  
>  // Replace all the nops in all the epilogues of asm.js functions with jumps
>  // to the profiling epilogues.

This comment needs an update.

::: js/src/asmjs/WasmFrameIterator.h
@@ +113,5 @@
>  
>  // Runtime patching to enable/disable profiling
>  
>  void
> +EnableProfiling(const Module& module, const CallSite& callSite, bool enabled);

Pre-existing, but it feels weird that these functions have "Enable" in their names and also take a boolean meaning "do we want to enable or disable", so calling EnableProfiling can effectively disable profiling. How do you feel about the name "ToggleProfiling", with the same signature?

::: js/src/asmjs/WasmIonCompile.cpp
@@ +901,3 @@
>          }
>  
> +        return callPrivate(callee, args, mg_.sigs[sigIndex].ret(), def);

It feels a bit weird to change the signature of funcPtrCall to take the sigIndex rather than the Sig, and then do a lookup of the Sig here. Can we revert to passing the Sig to funcPtrCall so we don't have this lookup?

::: js/src/asmjs/WasmStubs.h
@@ +39,5 @@
>  extern Offsets
>  GenerateInterruptStub(jit::MacroAssembler& masm);
>  
> +extern Offsets
> +GenerateTableThunk(jit::MacroAssembler& masm, uint32_t target);

This is never defined and can be removed.

::: js/src/asmjs/WasmTypes.h
@@ +429,4 @@
>                               uint32_t profilingJump = 0,
>                               uint32_t profilingEpilogue = 0)
>        : ProfilingOffsets(),
> +        tableEntry(tableEntry),

Can we initialize tableProfilingJump too, to make Coverity et al. happy?

@@ +434,5 @@
>          profilingJump(profilingJump),
>          profilingEpilogue(profilingEpilogue)
>      {}
>  
> +    // Function CodeRanges have a table entry which takes an extra signature 

nit: trailing space

::: js/src/jit-test/tests/wasm/basic.js
@@ +469,5 @@
> +            (import $foo "f" "")
> +            (func (call_import $foo))
> +            (func (result i32) (i32.const 0))
> +            (table 0 1)
> +            (func $bar (call_indirect 0 (i32.const 0)))

Can you explicitly create the signature's type, and refer to it with a name reference here, rather than an implicit index, please?

@@ +473,5 @@
> +            (func $bar (call_indirect 0 (i32.const 0)))
> +            (export "" $bar)
> +        )`,
> +        {f:() => { stack = new Error().stack }}
> +    )();

Can you assert this returns undefined?

::: js/src/jit-test/tests/wasm/profiling.js
@@ +86,5 @@
> +{
> +    enableSPSProfiling();
> +    var f = wasmEvalText(code);
> +    enableSingleStepProfiling();
> +    assertThrowsInstanceOf(() => f(), error);

uber nit, `() => f()` is equivalent to `f`, here

::: js/src/jit/shared/CodeGenerator-shared.cpp
@@ +1520,5 @@
>          break;
> +      case MAsmJSCall::Callee::Dynamic: {
> +        Register ptr = ToRegister(ins->getOperand(mir->dynamicCalleeOperandIndex()));
> +        if (callee.dynamicHasSigIndex()) {
> +            MOZ_ASSERT(ptr == WasmTableCallPtrReg);

We have a useFixed for this register for all the dynamic calls right? So this assertion could be hoisted up one level? (bonus: we'd remove the {} for the `if` body then)

Comment 17

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #16)
Good points!

> ::: js/src/asmjs/WasmIonCompile.cpp
> It feels a bit weird to change the signature of funcPtrCall to take the
> sigIndex rather than the Sig, and then do a lookup of the Sig here. Can we
> revert to passing the Sig to funcPtrCall so we don't have this lookup?

Well, we'd still need to pass the sigIndex b/c it's a parameter to MAsmJSCall::Callee() above, so the question is whether we should path *both* the index and Sig& or just the index.  It seemed more concise to just pass the index.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ca82dcfc2a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7d5e4ca750
https://hg.mozilla.org/integration/mozilla-inbound/rev/520c9c228706
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a0b729a7be
https://hg.mozilla.org/integration/mozilla-inbound/rev/d21a912dfd85

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ca82dcfc2a3
https://hg.mozilla.org/mozilla-central/rev/ff7d5e4ca750
https://hg.mozilla.org/mozilla-central/rev/520c9c228706
https://hg.mozilla.org/mozilla-central/rev/d9a0b729a7be
https://hg.mozilla.org/mozilla-central/rev/d21a912dfd85

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

I think this can be closed as resolved fixed now?

Comment 21

[Luke Wagner [:luke]]

Correctamundo