WebAssembly.Memory "index: u32/u64" constructor parameter #39
Conversation
|
Sounds like |
|
No, it's even better than my proposal. Will update the PR |
dakenf
changed the title
|
I've fixed V8 to work with 64bit memory and threads. Except changes in WebAssembly.Memory there was also an issue with compiler that prevented passing i64.const to memory.fill and memory.init. I think I will clean up the code and make a PR in next few days. Can we get this reviewed and merged? I'm glad to make any changes to get it done |
|
It looks like your CL to add Memory index type support in the JS API to V8 is your first contribution to V8 (https://crrev.com/c/4742982). Anyway, we should land this PR before adding support in V8. Can you also add spec tests for the new parameter please? |
|
It was my first contribution, will add the tests today |
|
I've added JS tests. There were memory64 tests in test/core/bulk64._wast but they are disabled because the interpreter does not support memory.copy, fill and init operations. And as it's written in ocaml, i don't think i can add these op support in a reasonable time |
| const argument = { "initial": 1, "index": "u64" }; | ||
| const memory = new WebAssembly.Memory(argument); | ||
| assert_Memory(memory, { "size": 1 }); | ||
| assert_equals(memory.type().index, "u64"); |
There was a problem hiding this comment.
This seems to depend on the js-types proposal: https://github.com/WebAssembly/js-types/blob/main/proposals/js-types/Overview.md . Is that intentional? I guess we should probably not depend on that as it is not complete yet.
There was a problem hiding this comment.
I don't think so. It's a WebAssembly.Memory method that returns "memory type" object:
var memory = new WebAssembly.Memory({ initial: 1, index: "u64"})
memory.type(){minimum: 1, shared: false, index: 'u64'}
There was a problem hiding this comment.
Well, you are right. It's a part of the proposal. Should i remove these tests then? I guess i can keep "Unknown memory index" and just test that it creates something with different "index" arguments
There was a problem hiding this comment.
I guess so yes, is there any other way to verify the type I wonder?
There was a problem hiding this comment.
Perhaps we could detect the presence of the type() method at runtime and leave a comment about how this depends on js-types proposal.
Also, I wonder if you could include the index check in assert_Memory so this could be a single line:
assert_Memory(memory, { "size": 1, "index": "u64" });
There was a problem hiding this comment.
You can check the index type by importing the memory as memory32 or memory64 and check which of those works.
There was a problem hiding this comment.
I guess so yes, is there any other way to verify the type I wonder?
Compile a small wasm test with 64bit memory.init, embed it into JS as UInt8Array and try running with that memory. But it looks like an overkill. WebAssembly.Memory does not export any other info to JS
There was a problem hiding this comment.
There's also a slight problem with checking type() as i did not think it's part of another proposal and updated V8 code to include the "index" field. So to include that, js-types proposal should be updated
There was a problem hiding this comment.
I guess it will be part of whichever proposal lands first.
|
I went with the easy way, checked presence of type() method and updated assert_Memory to check the index value |
| }, "Shared memory with u32 index constructor"); | ||
|
|
||
| test(() => { | ||
| const argument = { "initial": 1, "maximum": 2, "index": "u64", shared: true }; |
There was a problem hiding this comment.
I think shared: true also comes from a spec that has yet to land (https://github.com/WebAssembly/threads) so I don't think we should test that here.
|
Oh wait, I think we should have gone with |
|
Sorry to flip flop on this.. I know I suggested 'u64' back in #39 (comment) but I think I was wrong. |
|
Yeah, why not. I think i'm the only one who's using it right now so it won't hurt anyone. Will make a new changeset to V8 |
Similar to WebAssembly.Memory (WebAssembly#39), the WebAssembly.Table constructor also needs an `index` argument.
Similar to WebAssembly.Memory (#39), the WebAssembly.Table constructor also needs an `index` argument.
Similar to WebAssembly.Memory (WebAssembly#39), the WebAssembly.Table constructor also needs an `index` argument.
Currently spec does not allow creating 64 bit memory from JS code, so there is no way to create a shared memory instance with more than 65,536 pages.
I propose to add an optional parameter that would create a 64 bit memory instance without those limits.