-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
js-api: Add implementation defined limit of 16GiB for 64-bit memories. #100
Conversation
why limit it to 16GiB? pretty reasonable programs can use waay more memory than that... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
We need some spec'ed limit for Web embeddings to ensure predictability on the Web. We can always decide to raise it later, when the need arises. Any allocation above 16GiB is very unlikely to succeed on many clients anyway, and 16GiB is also the limit that the V8 sandbox currently imposes in Chrome. |
@@ -1781,7 +1781,8 @@ In practice, an implementation may run out of resources for valid modules below | |||
|
|||
<ul> | |||
<li>The maximum size of a table is 10,000,000.</li> | |||
<li>The maximum number of pages of a memory is 65,536.</li> | |||
<li>The maximum size of a 32-bit memory is 65,536 pages (4 GiB).</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually necessary, given that more than 4G isn't even addressable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should just drop this line then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its somewhat useful to have the two limits listed next to each other like this.. even though I guess its redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nevermind, the limit itself is pre-existing.
@sbc100 The implementation limit in Firefox currently stands at 8GiB, not 16. I think we'd be open to raising that limit, but the relevant people are on PTO right now so we'll have to get back to you later. In the meantime, we'd prefer that the spec say 8GiB. |
Oh, from https://searchfox.org/mozilla-central/source/js/src/wasm/WasmConstants.h#1156 we concluded that Firefox doesn't have any limit currently. Where is the 8GiB limit implemented? |
Ah that makes sense, sorry about the confusion. That constant is just for validation, not implementation. Here's the implementation limit: https://searchfox.org/mozilla-central/source/js/src/wasm/WasmMemory.cpp#253 |
Thanks for clarifying, that makes sense! We heard from partners that an 8GiB limit wouldn't be hugely helpful though given that a wasm64 program by design always needs more memory than the corresponding wasm32 program. So the 8GiB won't give you substantially more memory than the 4GiB for wasm32. |
This was spec'ed in WebAssembly/memory64#100. Also assert that the spec'ed size matches the supported size on 64-bit architectures. [email protected] Bug: 41480462 Change-Id: I1b6d08d7cfebc7c6ced71408371e8add2ed9c36c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6003067 Commit-Queue: Clemens Backes <[email protected]> Reviewed-by: Eva Herencsárová <[email protected]> Cr-Commit-Position: refs/heads/main@{#97073}
We'll look into it on our side. I don't know if there's anything fundamental about it, or it's just conservative. Some relevant folks are on PTO right now, so it'll take a while. If there is something fundamental about it, we may ask that the implementation limit only grow to 8 instead of 16. |
We think we'll be able to increase the limit to 16GiB. We're going to do the work here [1]. |
No description provided.