[wasm-gc] Basic JS interop handling for wasm objects [v8/v8 : main]
Matthias Liedtke (Gerrit)
Patch set 6:Commit-Queue +1
To view, visit change 3879616. To unsubscribe, or for help writing mail filters, visit settings.
Matthias Liedtke (Gerrit)
1 comment:
File src/builtins/promise-resolve.tq:
Patch Set #11, Line 164:
// Skip "then" lookup for Wasm objects as they are opaque.This could also already be done in line 121.
It might however be reasonable to consider this as part of the "slow" path as it is somewhat much less likely to have opaque wasm objects as a resolution value.
To view, visit change 3879616. To unsubscribe, or for help writing mail filters, visit settings.
Matthias Liedtke (Gerrit)
Patch set 15:Commit-Queue +1
7 comments:
File test/mjsunit/wasm/gc-js-interop.js:
// TODO(7748): This assertion doesn't hold true, as some cases run into
// deopt loops.
// assertTrue(%ActiveTierIsTurbofan(fn));How much do we care about it for the MVP?
Patch Set #15, Line 55:
// TODO: test repeated execution of functions using ICs, including optimized.As proposed I have changed everything to repeated execution.
The issue is that this is quite slow taking about 80-90 seconds in `debug` (7 seconds in `optdebug`).
As discussed offline, I'll split it into multiple files to allow parallel execution.Still, if there are some redundant test patterns (e.g. if we could condense all trigonometric functions into one because they use the same code paths), I'm happy to reduce it as well.
Patch Set #15, Line 282:
testThrowsRepeated(() => new Symbol(wasm_obj), TypeError);These tests just iterate over all these functions in the global namespace.
For some of them there are `TypeError`s with `new`, for some without.
So in those cases the corresponding tests actually don't do anything with the argument as the `new` operator already throws the `TypeError`.
Should we remove them from the test or just keep them "just in case"?// FIXME(mliedtke): Should we repeat the same testing for async generator
// functions as for the synchronous ones?Can we just assume that it's pretty much the same + promises?
I don't really want to bloat the test even more.Patch Set #15, Line 697:
repeated(() => assertTrue(Reflect.defineProperty(obj, "prop", wasm_obj)));This doesn't make any sense as the third parameter should be an object e.g. with a value element. Still it doesn't throw and just behaves like an empty object (see comment above).
Should we change that?Patch Set #15, Line 725:
let proxy = new Proxy(wasm_obj, handler);An alternative would be to fail early and preventing to install a wasm object as a handler. This however would require additional code and for now the implementation choice was "keep the changes minimal". 😊
// TODO(mliedtke): What about `export {struct, array}`?
// d8 doesn't seem to accept exports (as the file isn't a module?)Is there a simple way to test importing/exporting?
To view, visit change 3879616. To unsubscribe, or for help writing mail filters, visit settings.
Matthias Liedtke (Gerrit)
1 comment:
File test/mjsunit/wasm/gc-js-interop.js:
Patch Set #15, Line 55:
// TODO: test repeated execution of functions using ICs, including optimized.As proposed I have changed everything to repeated execution. […]
Done
To view, visit change 3879616. To unsubscribe, or for help writing mail filters, visit settings.
Matthias Liedtke (Gerrit)
Attention is currently required from: Jakob Kummerow.
Patch set 18:Commit-Queue +1
1 comment:
Patchset:
@Jakob: Sorry for the large CL. Mostly these are test cases, the changes in production code are small.
There are some comments on some cases whether the tested behavior is the desired one, so some further discussion might be required.
To view, visit change 3879616. To unsubscribe, or for help writing mail filters, visit settings.
Matthias Liedtke (Gerrit)
Attention is currently required from: Jakob Kummerow.
Patch set 19:Commit-Queue +1
Matthias Liedtke (Gerrit)
Attention is currently required from: Jakob Kummerow.
Patch set 20:Commit-Queue +1
Matthias Liedtke (Gerrit)
Attention is currently required from: Jakob Kummerow.
Patch set 21:Commit-Queue +1
Jakob Kummerow (Gerrit)
Attention is currently required from: Matthias Liedtke.
Patch set 21:Code-Review +1
27 comments:
Patchset:
Nice work, very thorough! LGTM with minor comments.
File src/builtins/promise-resolve.tq:
Patch Set #11, Line 164:
// Skip "then" lookup for Wasm objects as they are opaque.
This could also already be done in line 121. […]
Ack
File src/objects/js-objects-inl.h:
isolate->Throw(*isolate->factory()->NewTypeError(
MessageTemplate::kWasmObjectsAreOpaque));
return {};```
THROW_NEW_ERROR(isolate,
NewTypeError(MessageTemplate::kWasmObjectsAreOpaque),
HeapObject);
```
File src/objects/js-objects.cc:
case LookupIterator::WASM_OBJECT:
it->NotFound();
return it->isolate()->factory()->undefined_value();nit: this is the same as the JSPROXY case, so you could combine it with that.
if (value->IsWasmObject()) {
RETURN_FAILURE(isolate, should_throw,
NewTypeError(MessageTemplate::kWasmObjectsAreOpaque));
}I'm not convinced that we should throw at this point. Just wait until that prototype is encountered in some later lookup, it'll throw then :-)
File test/mjsunit/wasm/gc-js-interop-async.js:
Patch Set #21, Line 29:
assertTrue(False)nit: I think you mean `assertUnreachable()`. `False` is a Python-ism, or a `ReferenceError` in JS.
Patch Set #21, Line 41:
assertTrue(false)`assertUnreachable()`, again a few more times below.
Patch Set #21, Line 59:
asssignednit: `assigned`
File test/mjsunit/wasm/gc-js-interop-collections.js:
Int8Array, Int16Array, Int32Array, Uint8Array, Uint16Array,
Uint32ArrayIf we wanted to save some test execution time, we could limit ourselves to one of these, as they share their implementations anyway. But also fine to keep them all.
Patch Set #21, Line 157:
asssigned`assigned`
File test/mjsunit/wasm/gc-js-interop-global-constructors.js:
Patch Set #21, Line 27:
testThrowsRepeated(() => EvalError(wasm_obj), TypeError);dupe of the previous line
Patch Set #21, Line 67:
testThrowsRepeated(() => new EvalError(wasm_obj), TypeError);dupe
Patch Set #21, Line 95:
asssigned`assigned`, and a few more times in the other files
File test/mjsunit/wasm/gc-js-interop-helpers.js:
Patch Set #20, Line 5:
// Flags: --experimental-wasm-gc --wasm-gc-js-interop --allow-natives-syntaxYou don't need this if you add a SKIP line to mjsunit.status. (Executing this file as a test isn't useful, after all.)
Patch Set #20, Line 12:
function MakeInstance() {nit: this nested function doesn't really accomplish anything, so just inline it. (It previously kept its local variables out of the top-level scope, but that purpose is now served by `CreateWasmObjects`.)
File test/mjsunit/wasm/gc-js-interop-numeric.js:
Patch Set #21, Line 53:
new BigInt(123)`123n`
File test/mjsunit/wasm/gc-js-interop-objects.js:
Patch Set #21, Line 90:
Should this throw?No, just like the third argument of `Object.defineProperty`.
repeated(() => assertTrue(Reflect.defineProperty(obj, 'prop', wasm_obj)));
repeated(() => assertTrue(obj.hasOwnProperty('prop')));
repeated(() => assertEquals(undefined, obj.prop));
repeated(
() => assertTrue(
Reflect.defineProperty(obj, 'prop2', {value: wasm_obj})));
repeated(() => assertSame(wasm_obj, obj.prop2));nit: in the few cases where you have sequences of operations that build on each other's effects, it could make sense to combine them into a single function. So this entire block (lines 86 through 98) would become:
```
repeated(() => {
let obj = {};
assertTrue(Reflect.define...);
assertTrue(obj.hasOwn...);
...
});
```Patch Set #21, Line 103:
assertFalse(Reflect.has(wasm_obj, 'prop'));Is there a reason this isn't `repeated(...)`?
File test/mjsunit/wasm/gc-js-interop.js:
// TODO(7748): This assertion doesn't hold true, as some cases run into
// deopt loops.
// assertTrue(%ActiveTierIsTurbofan(fn));How much do we care about it for the MVP?
Not very much. For the MVP, it's important to have the right behavior and not crash; avoiding deopt loops is just a performance thing and hence can wait.
Patch Set #15, Line 282:
testThrowsRepeated(() => new Symbol(wasm_obj), TypeError);
These tests just iterate over all these functions in the global namespace. […]
I don't mind either way. It's fine to drop them.
(We *could* pass a third argument to `assertThrows` that checks the error message... would that be worth it?)
// FIXME(mliedtke): Should we repeat the same testing for async generator
// functions as for the synchronous ones?
Can we just assume that it's pretty much the same + promises? […]
Should be fine.
Patch Set #15, Line 697:
repeated(() => assertTrue(Reflect.defineProperty(obj, "prop", wasm_obj)));
This doesn't make any sense as the third parameter should be an object e.g. with a value element. […]
Not throwing is correct here.
`Reflect.defineProperty` performs `ToPropertyDescriptor` on the third argument, which carefully checks with `has` whether `value` etc exist before accessing them. We expect that [[Has]] returns `false` (without throwing) for Wasm objects, so passing `wasm_obj` here is indeed equivalent to passing an empty object `{}`.
You can include a summary of this explanation in the comment.
(See https://tc39.es/ecma262/#sec-topropertydescriptor .)
Patch Set #15, Line 725:
let proxy = new Proxy(wasm_obj, handler);
An alternative would be to fail early and preventing to install a wasm object as a handler. […]
I think allowing Wasm objects as Proxy targets is fine.
// TODO(mliedtke): What about `export {struct, array}`?
// d8 doesn't seem to accept exports (as the file isn't a module?)Is there a simple way to test importing/exporting?
d8 supports modules. See test/mjsunit/modules-* for inspiration. (And do it in a follow-up CL 😊)
File test/mjsunit/wasm/gc-js-interop.js:
{
wasm_obj.foo = 42;
}nit: didn't you find these prettier when they were one-liners? The joys of code formatting!
(You may keep them as they are if you wish.)also interesting: `assertFalse(2 === wasm_obj)`
and `assertFalse({} === wasm_obj)`
To view, visit change 3879616. To unsubscribe, or for help writing mail filters, visit settings.
Matthias Liedtke (Gerrit)
Attention is currently required from: Jakob Kummerow.
Patch set 22:Commit-Queue +1
21 comments:
File src/objects/js-objects-inl.h:
isolate->Throw(*isolate->factory()->NewTypeError(
MessageTemplate::kWasmObjectsAreOpaque));
return {};
``` […]
Done
File src/objects/js-objects.cc:
case LookupIterator::WASM_OBJECT:
it->NotFound();
return it->isolate()->factory()->undefined_value();nit: this is the same as the JSPROXY case, so you could combine it with that.
Done
if (value->IsWasmObject()) {
RETURN_FAILURE(isolate, should_throw,
NewTypeError(MessageTemplate::kWasmObjectsAreOpaque));
}
I'm not convinced that we should throw at this point. […]
As this requires some more changes to some of the prototype handling, I'll address this in another CL, so we can follow-up on it independently of this CL.
File test/mjsunit/wasm/gc-js-interop-async.js:
Patch Set #21, Line 29:
assertTrue(False)nit: I think you mean `assertUnreachable()`. `False` is a Python-ism, or a `ReferenceError` in JS.
Done
Patch Set #21, Line 41:
assertTrue(false)`assertUnreachable()`, again a few more times below.
Done
Patch Set #21, Line 59:
asssignednit: `assigned`
Done
File test/mjsunit/wasm/gc-js-interop-collections.js:
Patch Set #21, Line 157:
asssigned`assigned`
Done
File test/mjsunit/wasm/gc-js-interop-global-constructors.js:
Patch Set #21, Line 27:
testThrowsRepeated(() => EvalError(wasm_obj), TypeError);dupe of the previous line
Done
Patch Set #21, Line 67:
testThrowsRepeated(() => new EvalError(wasm_obj), TypeError);dupe
Done
Patch Set #21, Line 95:
asssigned`assigned`, and a few more times in the other files
Done
File test/mjsunit/wasm/gc-js-interop-helpers.js:
Patch Set #20, Line 5:
// Flags: --experimental-wasm-gc --wasm-gc-js-interop --allow-natives-syntax
You don't need this if you add a SKIP line to mjsunit.status. […]
Done
Patch Set #20, Line 12:
function MakeInstance() {
nit: this nested function doesn't really accomplish anything, so just inline it. […]
Done
File test/mjsunit/wasm/gc-js-interop-numeric.js:
Patch Set #21, Line 53:
new BigInt(123)`123n`
Done
File test/mjsunit/wasm/gc-js-interop-objects.js:
Patch Set #21, Line 90:
Should this throw?No, just like the third argument of `Object.defineProperty`.
Done
repeated(() => assertTrue(Reflect.defineProperty(obj, 'prop', wasm_obj)));
repeated(() => assertTrue(obj.hasOwnProperty('prop')));
repeated(() => assertEquals(undefined, obj.prop));
repeated(
() => assertTrue(
Reflect.defineProperty(obj, 'prop2', {value: wasm_obj})));
repeated(() => assertSame(wasm_obj, obj.prop2));
nit: in the few cases where you have sequences of operations that build on each other's effects, it […]
Done
Patch Set #21, Line 103:
assertFalse(Reflect.has(wasm_obj, 'prop'));
Is there a reason this isn't `repeated(... […]
Done
File test/mjsunit/wasm/gc-js-interop.js:
// TODO(7748): This assertion doesn't hold true, as some cases run into
// deopt loops.
// assertTrue(%ActiveTierIsTurbofan(fn));
Not very much. […]
Done
Patch Set #15, Line 282:
testThrowsRepeated(() => new Symbol(wasm_obj), TypeError);
I don't mind either way. It's fine to drop them. […]
Back then I decided not to do it mainly for readability and simplicity and I still feel inclined not to do it.
Patch Set #15, Line 697:
repeated(() => assertTrue(Reflect.defineProperty(obj, "prop", wasm_obj)));
Not throwing is correct here. […]
Done, thanks for the explanation. 😊
File test/mjsunit/wasm/gc-js-interop.js:
{
wasm_obj.foo = 42;
}
nit: didn't you find these prettier when they were one-liners? The joys of code formatting! […]
Done
also interesting: `assertFalse(2 === wasm_obj)` […]
Done
To view, visit change 3879616. To unsubscribe, or for help writing mail filters, visit settings.
Jakob Kummerow (Gerrit)
Attention is currently required from: Matthias Liedtke.
Patch set 22:Code-Review +1
Matthias Liedtke (Gerrit)
Patch set 22:Commit-Queue +2
Matthias Liedtke (Gerrit)
Attention is currently required from: Jakob Kummerow.
Patch set 23:Commit-Queue +1
Matthias Liedtke (Gerrit)
Attention is currently required from: Jakob Kummerow.
Patch set 24:Commit-Queue +1
Jakob Kummerow (Gerrit)
Attention is currently required from: Matthias Liedtke, Nico Hartmann.
Patch set 24:Code-Review +1
1 comment:
Patchset:
Still LGTM.
+Nico for src/compiler.
To view, visit change 3879616. To unsubscribe, or for help writing mail filters, visit settings.
Nico Hartmann (Gerrit)
Attention is currently required from: Matthias Liedtke.
Patchset:
src/compiler LGTM
To view, visit change 3879616. To unsubscribe, or for help writing mail filters, visit settings.
Matthias Liedtke (Gerrit)
Patch set 24:Commit-Queue +2
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
[wasm-gc] Basic JS interop handling for wasm objects
This change tests all JavaScript language constructs and builtins in
combination with the unwrapped Wasm objects.
For JavaScript, excluding some basic introspection (e.g.
`Object.isExtensible`) WebAssembly GC objects are treated opaque.
They can be passed around freely but don't allow any access to
properties, elements etc.
This behavior is currently exposed only if the `wasm-gc-js-interop`
flag is set.
Bug: v8:7748
Change-Id: If0dc368f99d4097e3eaf53edde4e244e3081e334
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3879616
Reviewed-by: Jakob Kummerow <jkum...@chromium.org>
Reviewed-by: Nico Hartmann <nicoha...@chromium.org>
Commit-Queue: Matthias Liedtke <mlie...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83299}
---
M src/builtins/base.tq
M src/builtins/promise-resolve.tq
M src/builtins/reflect.tq
M src/compiler/types.cc
M src/objects/js-objects-inl.h
M src/objects/js-objects.cc
M src/runtime/runtime-object.cc
M test/mjsunit/mjsunit.status
A test/mjsunit/wasm/gc-js-interop-async.js
A test/mjsunit/wasm/gc-js-interop-collections.js
A test/mjsunit/wasm/gc-js-interop-global-constructors.js
A test/mjsunit/wasm/gc-js-interop-helpers.js
A test/mjsunit/wasm/gc-js-interop-numeric.js
A test/mjsunit/wasm/gc-js-interop-objects.js
A test/mjsunit/wasm/gc-js-interop-wasm.js
M test/mjsunit/wasm/gc-js-interop.js
16 files changed, 1,063 insertions(+), 86 deletions(-)