Stream: git-wasmtime

Topic: wasmtime / PR #8616 Change `Tunables::static_memory_bound...

Wasmtime GitHub notifications bot (May 14 2024 at 15:28):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8616.

Wasmtime GitHub notifications bot (May 14 2024 at 15:28):

alexcrichton requested fitzgen for a review on PR #8616.

Wasmtime GitHub notifications bot (May 14 2024 at 15:28):

alexcrichton opened PR #8616 from alexcrichton:remove-another-page-based-variable to bytecodealliance:main:

This commit changes the wasm-page-sized static_memory_bound field to instead being a byte-defined unit rather than a page-defined unit. To accomplish this the field is renamed to static_memory_reservation and all references are updated. This builds on the support from #8608 to remove another page-based variable from the internals of Wasmtime.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (May 14 2024 at 15:56):

alexcrichton updated PR #8616.

Wasmtime GitHub notifications bot (May 14 2024 at 16:22):

abrown submitted PR review:

Looks good to me! I think consistently using bytes is just better; when I was mucking around in the pooling allocator it was always tricky to understand whether variables were page-based or byte-based. I remember having to handle a lot of overflow conditions which I found by proptest-ing... I assume that was handled somewhere else?

Wasmtime GitHub notifications bot (May 14 2024 at 16:26):

alexcrichton commented on PR #8616:

Yeah I think most of those were handled in https://github.com/bytecodealliance/wasmtime/pull/8608 although there's still memory_pages in the pooling allocator which I'd like to remove as well and is probably also a large source of overflow checks.

Wasmtime GitHub notifications bot (May 14 2024 at 16:35):

fitzgen commented on PR #8616:

I think consistently using bytes is just better

Agreed, especially since it is effectively necessary to support the custom-page-sizes proposal.

when I was mucking around in the pooling allocator it was always tricky to understand whether variables were page-based or byte-based

I think we should probably start newtyping our units. Way back when I wrote some stuff like this for wee_alloc and then someone pulled it out into a crate: https://docs.rs/memory_units

Don't think we want to use exactly that, but more that kind of thing.

Wasmtime GitHub notifications bot (May 14 2024 at 16:41):

abrown commented on PR #8616:

Don't think we want to use exactly that, but more that kind of thing.

I agree. That's inside the code base, but even from a CLI perspective I found it useful to use bytesize in the mpk.rs example to more easily just pass in strings like "4KiB" or whatever instead of having to do mental math.

Wasmtime GitHub notifications bot (May 14 2024 at 16:49):

alexcrichton merged PR #8616.

Last updated: Nov 22 2024 at 17:03 UTC