Adjust js-api spec's table implementation-defined limits #103
Conversation
|
I don't think we want to do this. The intent of adding table64 was to enable i64 function pointers, not to allow dramatically larger tables. In particular, in SpiderMonkey we have not relaxed our implementation limit on the actual size of tables, nor do we actually allow i64 tables larger than 2^32 items, as all tables are subject to the same runtime limit: https://searchfox.org/mozilla-central/source/js/src/wasm/WasmConstants.h#1150 It's important to recognize that this implementation-defined limit in the JS API has nothing to do with validation, even when constructing new tables via the JS API. |
document/js-api/index.bs
Outdated
| @@ -1780,7 +1778,7 @@ An implementation must throw a {{RuntimeError}} if one of the following limits i | |||
| In practice, an implementation may run out of resources for valid modules below these limits. | |||
|
|
|||
| <ul> | |||
| <li>The maximum size of a table is 10,000,000.</li> | |||
| <li>The maximal initial or maximum size of a table is 10,000,000.</li> | |||
There was a problem hiding this comment.
This is incorrect. This is a runtime condition; it's about the actual size of the table, not the limits.
|
I think the whole section is a bit unclear and needs clarification. Thanks for working on this, Eva! In the first section about CompileErrors we probably want to say that the maximum initial size is 10M. Otherwise the module is invalid. In the second section we mean that there is a dynamic limit of 10M; no table can ever grow bigger than that. This means that we return The declared maximum is not restricted by any of this. |
|
Our implementation throws a RuntimeError when constructing a Table through I do think the spec is confusing about this as it currently stands. I think the cases we need the JS API spec to cover, and be clearer about, are:
|
|
Eva uploaded a new commit; we tried to make the wording more clear and removed the mention of a |
|
@bvisness What's your opinion on this? If we merge this, then this test will need to be removed again: https://github.com/WebAssembly/memory64/blob/main/test/core/memory64.wast#L8 |
| @@ -1747,15 +1747,15 @@ Note: ECMAScript doesn't specify any sort of behavior on out-of-memory condition | |||
|
|
|||
| The WebAssembly core specification allows an implementation to define limits on the syntactic structure of the module. | |||
| While each embedding of WebAssembly may choose to define its own limits, for predictability the standard WebAssembly JavaScript Interface described in this document defines the following exact limits. | |||
| An implementation must reject a module that exceeds one of the following limits with a {{CompileError}}. | |||
| An implementation must reject a module that exceeds one of the following limits with a {{CompileError}}, i.e. the module is considered invalid. | |||
There was a problem hiding this comment.
Nitpick, but I don't think the module is really "considered invalid" in this situation. If you want to clarify the symmetry with the behavior for invalid modules, I would maybe say "as if the module was invalid".
There was a problem hiding this comment.
Hm, doesn't throwing CompileError indicate that the module is considered invalid? WebAssembly.validate would also return false if we exceed these limits, no?
The spec uses the module_validate condition to decide whether to throw an error or to return the compiled module (and the same condition is used for WebAssembly.validate).
module_validate again uses the terminology "if module is valid, ...".
So either the implementation-defined limits make modules invalid, or we need to extend the module_validate step to say something like "if module is valid and does not exceed implementation defined limits, ...".
There was a problem hiding this comment.
Hm, it seems there's actually quite a large discrepancy between engines on this point. I tested the module (module (table 1000000000 funcref)) and got the following:
$ js test.js
Is module valid? true
Does module compile? true
Does module instantiate? false
RuntimeError: too many table elements
$ d8 test.js
Is module valid? false
Does module compile? false
CompileError: WebAssembly.Module(): initial table size (1000000000 elements) is larger than implementation limit (10000000 elements) @+13
The fact that the spec demands a CompileError does seem to imply that we should at least be rejecting this module in new WebAssembly.Module and similar compilation paths. But, I find it very unpleasant that a perfectly valid wasm module would be rejected by WebAssembly.validate, since I'd expect WebAssembly.validate to be a thin proxy for module_validate (which would not return error).
Really the latter is the key point for me though: a module that exceeds the JS API implementation limits is still valid per the core wasm spec, and module_validate therefore wouldn't return error. Hence this confusion, I guess.
I think all this is outside the scope of this PR. I retract this complaint for now but I think I'll open another issue to discuss this confusion.
| <li>The maximum number of table entries in any table initialization is 10,000,000.</li> | ||
| <li>The maximum initial size of a 32-bit memory is 65,536 pages (4 GiB).</li> |
There was a problem hiding this comment.
It is impossible to request a 32-bit memory larger than 4GB, so this limit doesn't really mean anything. It should probably be removed.
| <li>The maximum size of a 32-bit memory is 65,536 pages (4 GiB).</li> | ||
| <li>The maximum size of a 64-bit memory is 262,144 pages (16 GiB).</li> | ||
| <li>A table can grow to have up to 10,000,000 entries.</li> | ||
| <li>A 32-bit memory can grow to have up to 65,536 pages (4 GiB).</li> |
There was a problem hiding this comment.
Again this limit doesn't mean anything; you can't have a 32-bit memory larger than 4GB. (I realize this one was already present, but I think it should be removed.)
|
I just realized that this is still on the memory64 repo. This should probably be migrated to the core spec repo if we still want to address it in this form. (Specifically targeting the |
|
Some changes touch lines that were only added in this repo, so part of it will have to stay in this repo. But I think it makes sense to first resolve WebAssembly/spec#1863 in the wasm-3.0 branch (thanks for opening that issue!), then rebase this repo and then potentially adjust the memory64-specific lines. |
Should we still be making changes in this repo now that its been merged into the upstream |
Oh, sorry, I didn't realize that this repo is already merged into |
|
@rossberg should we archive this repo at this point? (Or am I misunderstanding and can/should changes still be possible downstream?) |
|
I'm fine either way. It simplifies my life a bit if you do it on the wasm-3.0 branch, especially if there is potential for conflicts. But given that other repos also still have occasional activity, I have gone into the habit of doing regular upstreaming rounds. Though I would appreciate it if champions also created PRs for upstreaming when they do changes in other repos. |
|
Closing this PR since this proposal is done and the repo is being archived. Perhaps re-open it against wasm-3.0 in the spec repo. |
No description provided.