Baldr: add real heterogeneous function table

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected)

Details

Attachments

(7 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8749900 [details] [diff] [review]
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).
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8749900 - Flags: review?(bbouvier)
(Assignee)

Comment 2

2 years ago
Created attachment 8749901 [details] [diff] [review]
rename-callargs

Random cleanup while I was in the area: 'Call' is too broad of a name for this struct that encapsulates call args.
Attachment #8749901 - Flags: review?(bbouvier)
(Assignee)

Comment 3

2 years ago
Created attachment 8749905 [details] [diff] [review]
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 on attachment 8749901 [details] [diff] [review]
rename-callargs

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

Nice!
Attachment #8749901 - Flags: review?(bbouvier) → review+
Created attachment 8750798 [details] [diff] [review]
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)).
Attachment #8750798 - Flags: review?(luke)
Comment on attachment 8749900 [details] [diff] [review]
simplify-regs

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

Great! Less platform dependent code is always better.
Attachment #8749900 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 7

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

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

Yes, good point.
Attachment #8750798 - Flags: review?(luke) → review+
Keywords: leave-open
(Assignee)

Comment 10

2 years ago
Created attachment 8756701 [details] [diff] [review]
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.
Attachment #8756701 - Flags: review?(bbouvier)
(Assignee)

Comment 11

2 years ago
Created attachment 8756702 [details] [diff] [review]
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).
Attachment #8756702 - Flags: review?(bbouvier)
(Assignee)

Comment 12

2 years ago
Created attachment 8756705 [details] [diff] [review]
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.
Attachment #8756705 - Flags: review?(bbouvier)
Attachment #8756701 - Flags: review?(bbouvier) → review+
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.
Attachment #8756702 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 14

2 years ago
(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).
(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 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)
Attachment #8756705 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 17

2 years ago
(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.

Updated

2 years ago
Depends on: 1277449
I think this can be closed as resolved fixed now?
Flags: needinfo?(luke)
(Assignee)

Comment 21

2 years ago
Correctamundo
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(luke)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.