Stream: git-wasmtime

Topic: wasmtime / PR #3291 Refactor the internals of `Store<T>`

Wasmtime GitHub notifications bot (Sep 03 2021 at 15:04):

alexcrichton opened PR #3291 from changeup-opaque to main:

This commit is an overdue refactoring and renaming of some internals of
the Store type in Wasmtime. The actual implementation of Store<T>
has evolved from the original implementation to the point where some of
the aspects of how things are structured no longer makes sense. There's
also always been a lot of unnecessary gymnastics when trying to get
access to various store pieces depending on where you are in wasmtime.

This refactoring aims to simplify all this and make the internals much
easier to read/write. The following changes were made:

• The StoreOpaque<'_> type is deleted, along with the opaque()
  method.

• The StoreInnermost type was renamed to StoreOpaque.
  StoreOpaque<'_> is dead. Long live StoreOpaque. This renaming
  and a few small tweaks means that this type now suffices for all
  consumers.

• The AsContextMut and AsContext traits are now implemented for
  StoreInner<T>.

These changes, while subtly small, help clean up a lot of the internals
of wasmtime. There's a lot less verbose &mut store.as_context_mut().opaque() now. Additionally many methods can
simply start with let store = store.as_context_mut().0; and use things
internally. One of the nicer aspects of using references directly is
that the compiler automatically reborrows references as necessary
meaning there's lots of less manual reborrowing.

The main motivation for this change was actually somewhat roundabout
where I found that when StoreOpaque<'_> was being captured in closures
and iterators it's 3 pointers wide which is a lot of data to move
around. Now things capture over &mut StoreOpaque which is just one
nice and small pointer to move around. In any case though I've long
wanted to revisit the design of these internals to improve the
ergonomics. It's not expected that this change alone will really have
all that much impact on the performance of wasmtime.

Finally a doc comment was added to store.rs to try to explain all the
Store-related types since there are a nontrivial amount.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Sep 03 2021 at 15:08):

alexcrichton updated PR #3291 from changeup-opaque to main.

Wasmtime GitHub notifications bot (Sep 03 2021 at 15:08):

alexcrichton requested pchickey for a review on PR #3291.

Wasmtime GitHub notifications bot (Sep 03 2021 at 16:03):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Sep 03 2021 at 16:03):

bjorn3 created PR review comment:

Can this be pub(crate)?

Wasmtime GitHub notifications bot (Sep 03 2021 at 17:44):

alexcrichton updated PR #3291 from changeup-opaque to main.

Wasmtime GitHub notifications bot (Sep 03 2021 at 18:30):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Sep 03 2021 at 18:55):

alexcrichton merged PR #3291.

Wasmtime GitHub notifications bot (Sep 03 2021 at 18:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 03 2021 at 18:59):

alexcrichton created PR review comment:

Er forgot to answer this before I merged, but I don't think so. There's a lot of pub stuff already and I frequently get compiler errors about pub-crate things being in pub-interfaces, so I prefer to not deal with any of that.

Last updated: Nov 22 2024 at 16:03 UTC