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

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

6 months ago
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.)
Assignee

Updated

6 months ago
Blocks: 1488199
Assignee

Updated

6 months ago
Assignee

Updated

6 months ago
No longer blocks: 1488199
Assignee

Comment 1

6 months ago
Interacts with changes to WebAssembly.Table.prototype.set, so let's make it dependent.
Depends on: 1505768
Assignee

Comment 2

6 months ago
This simple change sits on top of the patches for anyref boxing and TypedObject support for anyref.
Attachment #9028646 - Flags: review?(luke)
Assignee

Comment 3

6 months ago
Update coming to deal with a backward-compat corner case.
Assignee

Comment 4

6 months ago
Some minor bug fixes (subtlety around default argument).
Attachment #9028646 - Attachment is obsolete: true
Attachment #9028646 - Flags: review?(luke)
Attachment #9028990 - Flags: review?(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.
Assignee

Comment 6

5 months ago
(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
(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.
(So far, positive reactions from littledan/Ms2ger on the issue.)
Assignee

Comment 9

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

Yeah, WILLFIX.
Assignee

Comment 10

5 months ago
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.
Attachment #9028990 - Attachment is obsolete: true
Attachment #9028990 - Flags: review?(luke)
Attachment #9032687 - Flags: review?(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)
Attachment #9032687 - Flags: review?(luke) → review+
Assignee

Comment 13

3 months ago

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.

Assignee

Comment 14

3 months ago

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?

Flags: needinfo?(Ms2ger)

(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.

Flags: needinfo?(Ms2ger)
Assignee

Comment 16

3 months ago

(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

3 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f42a0b6c8de
Let WebAssembly.Table.prototype.grow take a fill argument. r=luke
Assignee

Comment 18

3 months ago

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

Keywords: leave-open
Depends on: 1528770
Assignee

Comment 19

3 months ago

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.

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15448 for changes under testing/web-platform/tests
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)
Upstream PR was closed without merging

Comment 24

3 months ago
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc33c7516c3
WPTs for new WebAssembly.Table.prototype.grow.  r=ms2ger
Assignee

Updated

3 months ago
Keywords: leave-open
Upstream PR merged

Comment 26

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.