Stream: git-wasmtime

Topic: wasmtime / PR #7364 mpk: limit the number of protection keys

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:27):

abrown edited PR #7364:

This change stems from how slicing memory slots into MPK-protected regions limits the number of memories each store can access: e.g., with fifteen keys in use, a store only has access to a fifteenth of the available slots. If we simply multiple the number of memory slots needed to run the *.wast spec tests by fifteen, we run out of available memory. This limits the number of protection keys used to two, which still allows us to test the functionality without reserving too much memory.

Also, if Wasmtime is ever embedded in an application that also uses memory protection keys, it could be useful to limit how many Wasmtime allocates and uses.. This change not only limits the number of protection keys used at runtime, but takes that
further to attempt to limit the initial number of keys allocated. The unfortunate side effect of using a OnceLock is that the max setting is only applicable on the first invocation, the one that sets the OnceLock.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:27):

abrown requested alexcrichton for a review on PR #7364.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:27):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:27):

abrown created PR review comment:

Not sure about this?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:29):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:29):

abrown created PR review comment:

@alexcrichton, I'm not sure I like how the max parameter is really only a hint: it only applies the first time one calls keys.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:30):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 18:30):

abrown created PR review comment:

I guess I should just move this logic into keys if we end up using this approach.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 19:08):

alexcrichton submitted PR review:

Seems reasonable to me! I think this is fine to start out with at least and we can iterate on it over time. I do agree with what you mentioned though of pushing the handling of max directly into the keys function.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 19:08):

alexcrichton submitted PR review:

Seems reasonable to me! I think this is fine to start out with at least and we can iterate on it over time. I do agree with what you mentioned though of pushing the handling of max directly into the keys function.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 19:08):

alexcrichton created PR review comment:

This makes some sense to me in that you doubled the number of memories which would halve the limit of concurrent tests because right now each test creates its own engine.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 20:26):

abrown updated PR #7364.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 20:26):

abrown has marked PR #7364 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 20:26):

abrown requested wasmtime-core-reviewers for a review on PR #7364.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 20:27):

abrown has enabled auto merge for PR #7364.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 21:17):

abrown updated PR #7364.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 25 2023 at 22:14):

abrown merged PR #7364.

Last updated: Nov 22 2024 at 16:03 UTC