1507491 - JS API for Table.grow should take optional second argument for init value

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

The table.grow instruction takes a second argument, which is the initialization value.  The JS API should take that argument too.

There's no real spec for this yet, but basically the argument should be treated exactly like the Table.set argument, and a grow could be considered a grow-with-null-values followed by a sequence of sets.  (Or if you like, a fill.)

Comment 1

[Lars T Hansen [:lth]]

Interacts with changes to WebAssembly.Table.prototype.set, so let's make it dependent.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1507491-grow-takes-fill-argument.patch

This simple change sits on top of the patches for anyref boxing and TypedObject support for anyref.

Comment 3

[Lars T Hansen [:lth]]

Update coming to deal with a backward-compat corner case.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1507491-grow-takes-fill-argument-v2.patch

Some minor bug fixes (subtlety around default argument).

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 9028990 [details] [diff] [review]
bug1507491-grow-takes-fill-argument-v2.patch

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

Great, thanks!  A few suggestions/questions:

::: js/src/jit-test/tests/wasm/gc/tables-generalized.js
@@ +474,5 @@
> +    assertEq(t.get(t.length-1), ins.exports.f);
> +}
> +
> +// Bad function values as initializers must be ignored for backward
> +// compatibility reasons.

Do we know we need this for backcompat?  I'd be inclined to, when !defined(ENABLE_WASM_GC) throw on bad values and see if anyone complains.

::: js/src/wasm/WasmJS.cpp
@@ +2159,5 @@
>    return CallNonGenericMethod<IsTable, getImpl>(cx, args);
>  }
>  
> +static bool TableFill(JSContext* cx, Table* table, HandleValue fillValue,
> +                      uint32_t index, uint32_t limit, bool isGrow) {

I have to say: this TableFill function parameterized by isGrow, while achieving good code reuse, is pretty confusing when thinking through all the cases.  Obviously we must factor everything in the "if (value)" leading up to the setAnyFunc(), so how about hoisting *just that* into a function, duplicating the rest which will I think read better in the context of grow()/set().

@@ +2233,5 @@
> +  if (!ToTableIndex(cx, args.get(0), table, "set index", &index)) {
> +    return false;
> +  }
> +
> +  // index+1 is safe because table.length < UINT32_MAX

I think this invariant needs a MaxTableLength (in WasmConstants.h), checked in WasmTable.cpp (instead of UINT32_MAX) and static_assert'd to be <= UINT32_MAX.  Since there's already a MaxTableMaximumLength; how about we just rename/reuse that.  Doesn't seem valuable to have two different limits here.

@@ +2273,5 @@
>    }
>  
> +  // For backward compatibility reasons, (a) the fillValue argument must be
> +  // optional and (b) its default value must probably be null, though
> +  // technically it only needs to be null for function tables.

Having "for backward compatibility reasons" and "probably" in the comments seem like we're making some arbitrary choice, but I think we're simply implementing a spec here, so doesn't seem like the comment is necessary.

Comment 6

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #5)
> Comment on attachment 9028990 [details] [diff] [review]
> bug1507491-grow-takes-fill-argument-v2.patch
> 
> Review of attachment 9028990 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks!  A few suggestions/questions:
> 
> ::: js/src/jit-test/tests/wasm/gc/tables-generalized.js
> @@ +474,5 @@
> > +    assertEq(t.get(t.length-1), ins.exports.f);
> > +}
> > +
> > +// Bad function values as initializers must be ignored for backward
> > +// compatibility reasons.
> 
> Do we know we need this for backcompat?

One web-platform test fails, so in that sense yes.

> I'd be inclined to, when
> !defined(ENABLE_WASM_GC) throw on bad values and see if anyone complains.

wpt complains (the last test in grow.any.js, "Stray argument").  We can discuss how important this is but the API and the tests are out there.

Happy to discuss this but suggest we take the discussion here: https://github.com/WebAssembly/reference-types/issues/22

Comment 7

[Luke Wagner [:luke]]

(In reply to Lars T Hansen [:lth] from comment #6)
> > Do we know we need this for backcompat?
> 
> One web-platform test fails, so in that sense yes.

Ah, gotcha.  Well fortunately they're quite malleable :)

> Happy to discuss this but suggest we take the discussion here:
> https://github.com/WebAssembly/reference-types/issues/22

Will do, thanks.

Comment 8

[Luke Wagner [:luke]]

(So far, positive reactions from littledan/Ms2ger on the issue.)

Comment 9

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #8)
> (So far, positive reactions from littledan/Ms2ger on the issue.)

Yeah, WILLFIX.

Comment 10

[Lars T Hansen [:lth]]

Attachment: bug1507491-grow-takes-fill-argument-v3.patch

I believe this addresses all concerns from the previous round.  I also added a test case to WPT to test this, but we have plenty of test cases in the JS engine, so if my approach there is controversial I'd just as soon remove that and let ms2ger deal with that.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 9032687 [details] [diff] [review]
bug1507491-grow-takes-fill-argument-v3.patch

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

Looks good; r+ with these suggestions, but happy to discuss alternatives if they don't make sense.

::: js/src/wasm/WasmJS.cpp
@@ +2209,5 @@
>          return false;
>        }
>  
>        if (value) {
> +        MOZ_ASSERT(index < UINT32_MAX);

Can you expand into:
  MOZ_ASSERT(index <= MaxTableLength)
  static_assert(MaxTableLength < UINT32_MAX);

@@ +2269,5 @@
> +#ifdef ENABLE_WASM_GC
> +  if (args.length() > 1) {
> +    fillValue = args[1];
> +  }
> +#endif

To match what I proposed in the issue, it'd be good to eagerly (pre-ENABLE_WASM_GC) enact throwing behavior if a second arg is supplied for a table<anyfunc>.  To do that, I would think we'd unconditionally pull the fillValue out of args[1] and then only have the #ifdef in the TableKind::AnyFunction case below, simply wrapping the TableFunctionFill() call.  But actually, I don't see much risk or downside to just shipping this change to grow() right away, just to stake out the territory.

Independently, I was thinking that maybe we should introduce some intermediate ENABLE_WASM_REF_TYPES tier, so that we'd have:
 - ENABLE_WASM_BULKMEM_OPS, stuff shipping Real Soon
 - ENABLE_WASM_REF_TYPES, shipping when the reference types proposal gets to stage 4 (early-/mid-2019)
 - ENABLE_WASM_GC, after ref types

@@ +2271,5 @@
> +    fillValue = args[1];
> +  }
> +#endif
> +
> +  MOZ_ASSERT(oldLength <= UINT32_MAX - delta);

Similarly, could this be:
 MOZ_ASSERT(oldLength <= MaxTableLength - delta);
 static_assert(MaxTableLength < UINT32_MAX);

@@ +2279,5 @@
> +      RootedFunction value(cx);
> +      if (!fillValue.isNull() && IsExportedFunction(fillValue, &value)) {
> +        TableFunctionFill(cx, &table->table(), value, oldLength,
> +                          oldLength + delta);
> +      }

I think the logic here should be:
 if (fillValue.isNull()) {
    MOZ_ASSERT(table[i] is already null, nothing to do)
 } else if (IsExportedFunction(...)) {
    TableFunctionFill()
 } else {
    throw
 }

@@ +2291,5 @@
> +      if (!tmp.get().isNull()) {
> +        for (uint32_t index = oldLength; index < oldLength + delta; index++) {
> +          table->table().setAnyRef(index, tmp);
> +        }
> +      }

Maybe worth asserting the table elements already == tmp in an else case?

::: testing/web-platform/tests/wasm/jsapi/table/grow.any.js
@@ +49,5 @@
>  test(() => {
> +  const argument = { "element": "anyfunc", "initial": 1 };
> +  const table = new WebAssembly.Table(argument);
> +  const bin =
> +        new Uint8Array([0,97,115,109,1,0,0,0,1,4,1,96,0,0,3,2,1,0,7,6,1,2,102,110,0,0,10,4,1,2,0,11,]);

I think the wpt-wasm-wide convention is to build your module with the WasmModuleBuilder (you can see some simple examples in wasm/jsapi/module/*).  I think it should be pretty easy for the trivial module you want here.  Could you switch over to that and remove grow.any.wat?

@@ +103,5 @@
>    const table = new WebAssembly.Table(argument);
>    assert_equal_to_array(table, nulls(5), "before");
>  
> +  // A spurious third argument is ignored.
> +  const result = table.grow(3, null, {});

Could you also add a tests for:
 - two arguments, second is function, doesn't throw
 - two arguments, second is non-function, throws (with comment saying this is in preparation for future and link to the GH issue we discuss in)

Comment 12

[Lars T Hansen [:lth]]

Attachment: bug1507491-grow-takes-fill-argument-v4.patch

Rebased.

Comment 13

[Lars T Hansen [:lth]]

This is about to land.

For the documentation team:

   WebAssembly.prototype.grow(delta, [init])

where delta is an integer (as before) and init is an optional initialization value for new slots, defaulting to null. For tables of anyfunc (ie all tables at the moment) the init value can explicitly be null or a function that was exported by some Wasm module instance. If it is not, then this function throws.

Comment 14

[Lars T Hansen [:lth]]

There's a wrinkle here wrt WPT since we're technically changing an API:

• The existing test case in grow.any.js that tests that a bogus but unused second argument is allowed will succeed when the old table API is present but will fail when the new api is present (which is nightly-only for now)
• There will be new test cases that will fail when the old table API is present but will succeed when the new API is present

For the jit-tests I have all sorts of predicates to handle this, but how do i best handle it in wpt?

If I just land the changes to the tests they'll start failing when we merge to beta and/or upstream the tests, as the new APIs won't exist there. But can we handle that with .ini files on beta and can other browsers handle it with .ini files in their own repos, or is this too clunky?

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Lars T Hansen [:lth] from comment #14)

There's a wrinkle here wrt WPT since we're technically changing an API:

• The existing test case in grow.any.js that tests that a bogus but unused second argument is allowed will succeed when the old table API is present but will fail when the new api is present (which is nightly-only for now)

Changing the test to pass three arguments is fine; it's meant to fail if implementations have an argc == expected_args check rather than argc >= expected_args.

• There will be new test cases that will fail when the old table API is present but will succeed when the new API is present

Is there a spec for this at all? If not, I'd suggest adding a new file called foo.tentative.any.js, preferably with a pointer back to this bug or whatever place you're discussing the API.

For the jit-tests I have all sorts of predicates to handle this, but how do i best handle it in wpt?

If I just land the changes to the tests they'll start failing when we merge to beta and/or upstream the tests, as the new APIs won't exist there. But can we handle that with .ini files on beta and can other browsers handle it with .ini files in their own repos, or is this too clunky?

We can certainly use the release_or_beta predicate like https://searchfox.org/mozilla-central/rev/ee40541496d3ad738097eebadaf4965ca1343b7a/testing/web-platform/meta/worklets/audio-worklet-credentials.https.html.ini#6 for ourselves. Other browsers need to handle expected-to-fail tests anyway, so this shouldn't be a problem.

As for the stability-check orange, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1528735.

Comment 16

[Lars T Hansen [:lth]]

(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #15)

(In reply to Lars T Hansen [:lth] from comment #14)

There's a wrinkle here wrt WPT since we're technically changing an API:

• The existing test case in grow.any.js that tests that a bogus but unused second argument is allowed will succeed when the old table API is present but will fail when the new api is present (which is nightly-only for now)

Changing the test to pass three arguments is fine; it's meant to fail if implementations have an argc == expected_args check rather than argc >= expected_args.

Righto. So I'll just add a second null argument before the {} argument and we'll be fine.

• There will be new test cases that will fail when the old table API is present but will succeed when the new API is present

Is there a spec for this at all?

Sort of. It's emerging, and mostly stable, but I think we're the first to implement this bit.

If not, I'd suggest adding a new file called foo.tentative.any.js, preferably with a pointer back to this bug or whatever place you're discussing the API.

Will do.

For the jit-tests I have all sorts of predicates to handle this, but how do i best handle it in wpt?

If I just land the changes to the tests they'll start failing when we merge to beta and/or upstream the tests, as the new APIs won't exist there. But can we handle that with .ini files on beta and can other browsers handle it with .ini files in their own repos, or is this too clunky?

We can certainly use the release_or_beta predicate like https://searchfox.org/mozilla-central/rev/ee40541496d3ad738097eebadaf4965ca1343b7a/testing/web-platform/meta/worklets/audio-worklet-credentials.https.html.ini#6 for ourselves. Other browsers need to handle expected-to-fail tests anyway, so this shouldn't be a problem.

That will work very well.

As for the stability-check orange, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1528735.

Thanks for that, I'll point the sheriffs in that direction if somebody makes a fuss.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f42a0b6c8de
Let WebAssembly.Table.prototype.grow take a fill argument. r=luke

Comment 18

[Lars T Hansen [:lth]]

Landed w/o WPT tests since there are some issues with getting those to stick, hope to land them in a day or so.

Comment 19

[Lars T Hansen [:lth]]

Attachment: Bug 1507491 - WPTs for new WebAssembly.Table.prototype.grow. r?ms2ger

Test cases for evolving JSAPI for the wasm reference types proposal:
the grow() method on a table can now take a second argument. It can
be null or an exported wasm function and it is used to initialize the
new table slots.

These tests are set apart from the regular test to make it clear that
they are tentative. Once the reftypes proposal ships we should merge
them into table/grow.any.js.

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15448 for changes under testing/web-platform/tests

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/15448
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/Y9iORAuBQtqfda69VKqi1Q)

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6f42a0b6c8de

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc33c7516c3
WPTs for new WebAssembly.Table.prototype.grow.  r=ms2ger

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 26

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7fc33c7516c3

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/Table/grow needs update