1486549 - wasm: table.init: defer computation of code entry point as much as possible

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

[Julian Seward [:jseward]]

With the initial implementation of table.init that is part of bug 1471500, the
code entry point that will be used for a passive table initialiser is computed
at instantiation time.  By the time the owning instance gets around to
actually using those initialisers in a table.init instruction, there may be
more optimised entry points available.  It would be nice, therefore, to delay
the computation of entry points in this case until execution of table.init, so
that we can use the best one available at that point.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1486549-deferred-entrypts-1.diff

WIP patch, not really right.  Fails some regression tests.

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1486549-deferred-entrypts-3.diff

Rebased.  Still not right.  It MOZ_CRASHes in MetadataTier::lookupFuncExport
for some test cases, eg:

const Module = WebAssembly.Module;
const Instance = WebAssembly.Instance;

var m = new Module(wasmTextToBinary(`(module
    (table 3 anyfunc)
    (import $foo "" "foo" (result i32))
    (import $bar "" "bar" (result i32))
    (func $baz (result i32) (i32.const 13))
    (elem (i32.const 0) $foo $bar $baz)
    (export "foo" $foo)
    (export "bar" $bar)
    (export "baz" $baz)
    (export "tbl" table)
)`));
var jsFun = () => 83;
var wasmFun = new Instance(new Module(wasmTextToBinary(
    '(module (func (result i32) (i32.const 42)) (export "foo" 0))'
    ))).exports.foo;

var e1 = new Instance(m, {"":{foo:jsFun, bar:wasmFun}}).exports;

Comment 3

[Luke Wagner [:luke]]

Attachment: tidy-debug-cloning

I can take this.  Here's a preparatory patch that just moves a big hunk of code into its own function.

Comment 4

[Luke Wagner [:luke]]

Attachment: lazy-table-init (WIP)

(WIP that compiles, but I haven't started testing yet.)  As soon as I started hacking on this, I realized another problem with the initial MVP implementation that I had wanted to fix but forgot: sharing data/elem segments between modules and instances.  This ended up being pretty easy to address at the same time as changing ElemSegments to store code-range-indices by having every {Data,Elem}Segment be its own AtomicRefCounted, with the Module always holding a RefPtr, and the Instance holding a maybe-null vector of RefPtrs to its passive segments.  This way the segments go away when the module is GC'd and all its passive uses are dropped.

Comment 5

[Luke Wagner [:luke]]

Attachment: lazy-table-init

Now passing tests!

Comment 6

[Luke Wagner [:luke]]

Attachment: lazy-table-init (v.2)

Actually, as soon as I posted the last patch, I realized that the entire ElemSegment tiering situation is only to work around the lack of a function-index-to-code-range-index mapping in the MetadataTier.  In fact, debug-mode needs this so there's a debugFuncToCodeRange that grabs a copy of this Vector from the ModuleGenerator.  So this new version of the patch holds on to funcToCodeRange unconditionally (which is only a small fraction size increase) and, in exchange, a bunch of ElemSegment tiering logic goes away: ElemSegments just store the elemFuncIndices, nothing else, and become tier-invariant. \o/

Comment 7

[Julian Seward [:jseward]]

Comment on attachment 9008454 [details] [diff] [review]
tidy-debug-cloning

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

Looks OK to me.

::: js/src/wasm/WasmModule.cpp
@@ +1100,3 @@
>  
> +    SharedCode code(code_);
> +    if (metadata().debugEnabled && !(code = getDebugEnabledCode())) {

Nit: I had the impression that old C-style assignments within conditionals were frowned upon nowadays.  Although I can't see anything against it in the SM style guide.  Are you sure this will get past the various style-checkers on automation?

Comment 8

[Luke Wagner [:luke]]

(In reply to Julian Seward [:jseward] from comment #7)
I don't know that we have any hard static checks against this, but yeah, you're right; we do generally try to avoid this.  It's just so much more verbose the other way :/  Anyhow, a later patch (in bug 1487475) changes this control flow to want the nested if anyways, so I'll expand it here.

Comment 9

[Julian Seward [:jseward]]

Comment on attachment 9008577 [details] [diff] [review]
lazy-table-init (v.2)

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

Nice.  Looks like it reduces complexity and removes code.

Please add a short commit message which summarises the changes.  Maybe a recycled version of comment 6?

::: js/src/wasm/WasmInstance.cpp
@@ -445,4 @@
>  
> -    UniquePtr<DataSegmentInit>& segInit = dataSegInitVec[segIndex];
> -
> -    // Check that the segment is available for passive use.

I'd prefer to retain this comment.

@@ -452,5 @@
>                                   JSMSG_WASM_INVALID_PASSIVE_DATA_SEG);
>         return -1;
>      }
>  
> -    // If this is an active initializer, something is badly wrong.

And this one.  Ditto for analogous rm'd comments in other Instance::{mem,table}{Init,Drop} methods.

@@ +617,5 @@
> +            FuncImportTls& import = funcImportTls(funcImports[funcIndex]);
> +            JSFunction *fun = import.fun;
> +            if (IsExportedWasmFunction(fun)) {
> +                // This element is a wasm function imported from another
> +                // instance. To preserve semantic function identity, we must

What do you mean here and just below by "semantic function identity"?  Do you mean the same (func table index, callee instance) pairing, or something else?  Can you clarify this in the comment?

Comment 11

[Luke Wagner [:luke]]

(In reply to Julian Seward [:jseward] from comment #9)

From irc: agreed to keep those comments removed since the code says as much.

> > +                // This element is a wasm function imported from another
> > +                // instance. To preserve semantic function identity, we must
> 
> What do you mean here and just below by "semantic function identity"?  Do
> you mean the same (func table index, callee instance) pairing, or something
> else?  Can you clarify this in the comment?

You're right that's unclear; I meant the JS function identity (observed by === and observing expando properties).  Will expand to "To preserve the === function identity required by the JS embedding spec."

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35de475c93bc
Baldr: refactor/tidy up debug cloning of code (r=jseward)
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb950db9425b
Baldr: store only function indices in elem segments (r=jseward)

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/35de475c93bc
https://hg.mozilla.org/mozilla-central/rev/eb950db9425b