1500167 - Support multiple tables

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P2)

Description

[Lars T Hansen [:lth]]

Now that we have table-of-anyref and instructions to operate on those, generalize everywhere so that we can have more than one table.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1500167-multiple-tables.patch

Nearly complete, I'm running out of things to test.  But I think I left behind some minor FIXMEs that need attention.  And possibly I need some negative tests to test range checking.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1500167-multiple-tables-v2.patch

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1500167-multiple-tables-v3.patch

Added and adjusted several test cases; no functional changes.

Comment 4

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ac4f05cba6ee4606a6e1c6d98a74c1d26263822

Comment 5

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3071cc38b8e78f490ff722800d986ab62898ef62

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1500167-multiple-tables-v4.patch

Incorporate fixes for fairly obscure bugs discovered by try runs.

Comment 7

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4b3558aac9ad52e03c614b35720c926386fd2da

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 9023008 [details] [diff] [review]
bug1500167-multiple-tables-v4.patch

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

Glad to see things mostly sortof slotted into place, even though it was a rather large amount of total work.  Great job!

::: js/src/wasm/WasmConstants.h
@@ +588,5 @@
>  // These limits are agreed upon with other engines for consistency.
>  
>  static const unsigned MaxTypes               =  1000000;
>  static const unsigned MaxFuncs               =  1000000;
> +static const unsigned MaxTables              =   100000;  // NOT IN ANY SPEC!

nit: The uppercase makes it look like we're doing something bad here ;)  How about a lower case "// TODO: get in shared limits spec"?

::: js/src/wasm/WasmInstance.cpp
@@ +1328,5 @@
> +    // Ergo we must go through the entire list of tables in the instance here
> +    // and check for the table in all the cached-data slots; we can't exit after
> +    // the first hit.
> +
> +    for (uint32_t i=0; i < tables_.length(); i++) {

prediction: in 10 years, O(n) loop will show up as part of some O(n^2) pathological testcase and we'll have to register (Instance*,tableIndex) pairs on the Table like we do for Instance::deoptimizeImportExit() :P

::: js/src/wasm/WasmModule.cpp
@@ +1278,3 @@
>      SharedTableVector tables;
> +    uint32_t tableIndex = 0;
> +    for (const TableDesc& td : metadata().tables) {

nit: Module::instantiate() is a bit large and I try to keep it a high-level sequence of operations, so could you perhaps reintroduce instantiateTable() and push the loop into that (similar to, e.g., instantiateGlobals())?  Given that instantiate{Imported,Local}Table share a tail, I think it'd be fine to just inline them into instantiateTable().

::: js/src/wasm/WasmModule.h
@@ +91,5 @@
> +    bool instantiateImportedTable(JSContext* cx,
> +                                  const TableDesc& td,
> +                                  Handle<WasmTableObject*> table,
> +                                  WasmTableObjectVector& tableObjs,
> +                                  SharedTableVector* tables) const;

nit: could you take 'tableObjs' by pointer (as the outparam convention)?

::: js/src/wasm/WasmTable.h
@@ +31,5 @@
>  //
> +// A table of AnyFunction holds FunctionTableElems, which are (instance*,index)
> +// pairs, where the instance must be traced.  An earlier optimization avoiding
> +// the instance* for local tables does not interact well with multiple tables
> +// and the table.copy instruction.

nit: While it's true, the latter sentence seems like it'll be get increasingly stale (if not clobbered by bug 1340235) so I'd leave it out.

Comment 9

[Lars T Hansen [:lth]]

Attachment: bug1500167-multiple-tables-v5.patch

This addresses all comments and further fleshes out the stress test case.  I did not fold the two instantiate routines out into the now broken-out instantiate loop because the logic is clearer the way it is.  I have however cleaned up the representation of the function array further since there's a single representation now.  And I've simplified the API to Table somewhat; the arrays are no longer available externally to the Table code.

Carrying the r+, though there's some fresh code here.

Comment 10

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02a0574f9a8ef6e07d582a2596609a29c1b5acab

Comment 11

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea1da7c247ea61c752334deca1c2c060eeb971e8
Bug 1500167 - Support multiple tables in wasm. r=luke

Comment 12

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/ea1da7c247ea

Comment 13

[Chris Mills [:cmills]]

Need to update table descriptions in the docs in light of this.

Comment 14

[Lars T Hansen [:lth]]

Note this is nightly-only until the feature has been fully specced.

Comment 15

[Chris Mills [:cmills]]

Thanks LTH; I'll not add a note to the Fx65 rel notes, but I'll document this when I get ot the actual docs stage.