Skip to content
This repository was archived by the owner on Sep 8, 2025. It is now read-only.

remove StoreContextMut::with_{a,de}tached_instance[_async]#183

Merged
alexcrichton merged 1 commit into
mainfrom
dicej/safer-instances-part2
Jun 4, 2025
Merged

remove StoreContextMut::with_{a,de}tached_instance[_async]#183
alexcrichton merged 1 commit into
mainfrom
dicej/safer-instances-part2

Conversation

@dicej
Copy link
Copy Markdown
Collaborator

@dicej dicej commented Jun 3, 2025

This is the second phase of the cleanup I started in an earlier commit with the goal of reducing unsafe code in the concurrent module and submodules, especially with regard to functions which take mutable references to a store and a ComponentInstance within that store.

My earlier attempt to make such functions safe was to temporarily "detach" the instance from the store to avoid aliasing hazards, but that got messy and confusing, and Alex and I decided an index-based approach made more sense. So now we pass Instance handles around instead of ComponentInstance references and pointers, using them to retrieve the ComponentInstance from the store only as necessary.

Practically speaking, this required moving a lot of inherent functions from ComponentInstance to Instance and updating their bodies accordingly. Functionally, nothing has changed.

This is the second phase of the cleanup I started in an earlier commit with the
goal of reducing `unsafe` code in the `concurrent` module and submodules,
especially with regard to functions which take mutable references to a store and
a `ComponentInstance` within that store.

My earlier attempt to make such functions safe was to temporarily "detach" the
instance from the store to avoid aliasing hazards, but that got messy and
confusing, and Alex and I decided an index-based approach made more sense.  So
now we pass `Instance` handles around instead of `ComponentInstance` references
and pointers, using them to retrieve the `ComponentInstance` from the store only
as necessary.

Practically speaking, this required moving a lot of inherent functions from
`ComponentInstance` to `Instance` and updating their bodies accordingly.
Functionally, nothing has changed.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@alexcrichton alexcrichton added this pull request to the merge queue Jun 4, 2025
@alexcrichton
Copy link
Copy Markdown
Member

Very nice, thanks again for pushing through this!

Two minor comments:

  • We probably want to keep an eye out for ergonomics here as some of the concoctions of .0 or .store_opaque_mut() or such can get pretty wordy. I'll try to go over this tomorrow and see if I have any ideas.
  • Initially I was surprised about the use of Instance in most places as opposed to &Instance. That's often an antipattern in Rust b/c it can copy quite a few bytes, but here Instance is so small that it looks like it gets passed in registers on 64-bit platforms anyway so seems reasonable to me.

Merged via the queue into main with commit a3a5f3e Jun 4, 2025
44 checks passed
@alexcrichton alexcrichton deleted the dicej/safer-instances-part2 branch June 4, 2025 00:49
@dicej
Copy link
Copy Markdown
Collaborator Author

dicej commented Jun 4, 2025

Yeah, I went back and forth about &Instance vs Instance. I'm fine with whatever.

Also agreed about the ergonomics of looking up ComponentInstance. One thing that occurred to me is to define a trait that StoreContextMut, &mut StoreOpaque, and &mut dyn VMStore all implement and make Instance::instance take a generic param bounded by that trait. i.e. Rust-style "overloading".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants