Skip to content
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

Put MemoryPool and MmapMemory mmaps behind Arc #9682

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Nov 26, 2024

In upcoming work we're going to centralize memory management inside Mmap. In order to do that, we need memory that logically borrows from the Mmap to have access to it. That turns out to be possible to do by passing in &Mmap in some situations but very difficult in others (see #9681 for an attempt that was a real hassle to get working and still had a bunch of incomplete questions within it).

To prepare for this work, start moving these uses to be behind Arc. These aren't cloned at the moment, but they will be in the future.

In upcoming work we're going to centralize memory management inside `Mmap`.  In
order to do that, we need memory that logically borrows from the `Mmap` to have
access to it. That turns out to be possible in some situations but very
difficult in others (see bytecodealliance#9681 for an attempt that was a real hassle to get
working).

To prepare for this work, start moving these uses to be behind `Arc`. These
aren't cloned at the moment, but they will be in the future.
@sunshowers sunshowers requested a review from a team as a code owner November 26, 2024 08:04
@sunshowers sunshowers requested review from pchickey and removed request for a team November 26, 2024 08:04
/// reference. Making the callback `'static` goes some way towards ensuring
/// that, but it's still possible to squirrel away a reference into global
/// state. So don't do that.
unsafe fn with_slice_mut(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was somewhat annoying but I think this approach makes sense -- basically ensure that any &mut references are very short-lived as required by Rust's aliasing rules.

@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Nov 26, 2024
@alexcrichton
Copy link
Member

I think this can make sense but I'm having a hard time understanding the end goal without the second piece here of using the Arc itself. Would you be ok delaying merging this until the second half is in place? It'd be easier for me to review once that's there (I'm ok reviewing the two together as two separate commits myself)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants