Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use bytes for maximum size of linear memory with pooling #8628
Use bytes for maximum size of linear memory with pooling #8628
Changes from 1 commit
f0bf153
e07c350
e840ced
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This should be
65536
right? If you instead want the max address it should be10 * 65536 - 1
.Also lets use named constants for the Wasm page size so that it is easier to find uses and update in the future for the custom-page-sizes proposal. We have it defined in
wasmtime_types
already, FWIW, and this could use that here:wasmtime/crates/types/src/lib.rs
Line 1493 in c477424
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.
Ah yeah 65535 is a typo here, and everywhere else, but I'm hesitant to go through all the work to import a constant in all these places since it's all in tests anyway and all the values are basically random constants. In the future the wasm page size won't be constant as well so it feels weird to proliferate usage of the constant... I can change these to
>>16
and<<16
though where appropriate perhaps to reduce the risk of typos.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.
ditto regarding named constant
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.
Also confused about
65535
vs65536
here.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.
And ditto about named constants on everything here.
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.
Also confused about
65535
here since that isn't a multiple of the page size, so this will disallow any non-zero-length memories, assuming we don't do rounding elsewhere in this PR?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.
named constant, even tho it's just a test. we want to make sure we are actually testing sizes that wasm could potentially grow to, and not accidentally just testing rounding behavior or something.
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.
Could you expand on this? I could see a comment going here but I'm not sure otherwise what you're looking for in terms of a constant?
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.
If the goal is to test behavior with N wasm pages of capacity, then we should make sure we are actually doing that (as opposed to testing with unaligned capacity) and using a constant helps in this scenario. I feel like the typos are evidence that we can mess this up.
Additionally, having a constant that we can grep for all uses of will make it easier to support the custom-page-sizes proposal down the line: we have a list of uses that we can turn into method calls if necessary, and a list of tests that we might want to update or copy and tweak.
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.
The meaning of this test has sort of been updated and lost a bit over time as I've been tweaking settings. This isn't specifically testing for unalignment or anything like that it's just one size being bigger than the other and various bits and pieces have been lost to translation over time.
I'm gonna go ahead and merge this as-is but we can of course update tests afterwards. I'm basically just somewhat doubtful if it's worth the effort to try to document all tests for precisely what they're doing page-wise and such. I don't think any of the tests really need to be updated with custom-page-sizes since they're all using wasm memories of 64k pages and the tests for custom-page-sizes are going to be pretty orthogonal anyway. Much of the time the page limits are just to reduce VM memory reservations anyway and don't have much to do with page sizes.
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.
ditto
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.
ditto in this file's changes
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.
okay I am going to stop saying "ditto about named constants" now and assume you will go over the diff yourself to catch all of them at this point