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

Replace GetHost with a function pointer#158

Merged
alexcrichton merged 3 commits into
bytecodealliance:mainfrom
alexcrichton:accessor-refactor
May 9, 2025
Merged

Replace GetHost with a function pointer#158
alexcrichton merged 3 commits into
bytecodealliance:mainfrom
alexcrichton:accessor-refactor

Conversation

@alexcrichton
Copy link
Copy Markdown
Member

@alexcrichton alexcrichton commented May 8, 2025 •
edited
Loading

Major refactor:

  • Delete GetHost entirely.
  • Add a new HasData trait to replace it (see its own docs for more words)
  • Reimplement lots of wasmtime-wit-bindgen
  • Concurrent functions now get their own trait, meaning there's Host and, optionally, HostConcurrent
  • GetHost is effectively replaced with a normal function pointer, and D: HasData is used in its stead to handle the return value.
  • Accessor stores fn(...), no longer has a G
  • Accessor now has a D, but doesn't actually store it, just uses D::Data<'_> to assign a type to the function pointer
  • Access no longer derefs to D::Data, instead it requires a get() method. Lifetimes are subtly different which required refactoring host code
  • add_to_linker_get_host is all removed, only add_to_linker remains
  • Non-concurrent usage of bindgen! now has to worry about HasData, what I consider a small cost to pay for supporting all this
  • Draft wasi-http support is a bit wonky, but it works. Probably will want to workshop this more in the future.
  • No more need to type-annotate closures.
  • Lots of internal refactorings in wit-bindgen to make the concurrent/not trait split reasonable to manage

@alexcrichton
Move the GetHost trait used in bindgen! into Wasmtime
f9669a0 
Turns out we don't actually need to generate this `GetHost` trait, we
can instead have it live in one location with extra documentation. There
are already extra bounds on the `Host` associated type at all call-sites
so there's no need to additionally have trait bounds in the trait
definition, meaning the trait definition is always the same and it can
move within Wasmtime.

This shouldn't have any impact on any embedders today, it's just moving
things around.
@alexcrichton alexcrichton force-pushed the accessor-refactor branch 4 times, most recently from f8dff07 to 06c49de Compare May 9, 2025 18:40
@alexcrichton alexcrichton changed the title Accessor refactor Replace GetHost with a function pointer May 9, 2025
@alexcrichton alexcrichton marked this pull request as ready for review May 9, 2025 19:28
@alexcrichton alexcrichton requested a review from dicej May 9, 2025 19:28
@alexcrichton
Copy link
Copy Markdown
Member Author

alexcrichton commented May 9, 2025

Ok @dicej I think this is ready to go. Should be passing all tests and I've updated a few things:

  • There's now a description on this PR which gives a high-level overview
  • I wrote a bunch of docs on HasData which details various usage patterns throughout libraries we have
  • Tests should all be passing

I'd like to get your eyes on these changes as well, especially the ones related to Accessor. My plan would be to work through anything remaining with you and, pending that, then upstreaming everything next. Or at least upstreaming everything as much as possible without upstreaming p3. Basically this creates a major diff relative to main and I want to quickly burn that back down.

dicej
dicej approved these changes May 9, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dicej dicej left a comment

Choose a reason for hiding this comment

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

Looks great; safer and way less boilerplate. Thanks for putting so much thought and effort into this!

Comment thread crates/misc/component-async-tests/http/src/lib.rs
// TODO: once access to the store is possible in a non-async context (similar to Accessor pattern)
// we should convert this to a sync function that works w/ &mut self.
async fn finish<U>(
accessor: &mut Accessor<U, Self>,
async fn finish<T>(
Copy link
Copy Markdown
Collaborator

@dicej dicej May 9, 2025

Choose a reason for hiding this comment

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

Could we convert this to a sync function that takes a Access now, or would that require further wit-bindgen changes?

Copy link
Copy Markdown
Member Author

@alexcrichton alexcrichton May 9, 2025

Choose a reason for hiding this comment

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

Not yet, would require more wit-bindgen changes yeah. Would be pretty easy to do after plumbing the macro configuration, and the only real gotcha I see is that HostConcurrent is no longer a great name for the trait necessarily. That's not a huge thing though and is something we can massage over time too.

Comment thread crates/wasmtime/src/runtime/component/has_data.rs Outdated
Comment thread crates/wasmtime/src/runtime/component/has_data.rs Outdated
@alexcrichton
Copy link
Copy Markdown
Member Author

alexcrichton commented May 9, 2025

Ok I'm going to flag this for merge with follow-up work being:

That should hopefully prove end-to-end that this is all working and should proceed upstream as well.

@alexcrichton alexcrichton enabled auto-merge May 9, 2025 21:26
@alexcrichton alexcrichton added this pull request to the merge queue May 9, 2025
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request May 9, 2025
@alexcrichton
Add T: 'static to `Store<T>
491edf6 
Since the beginning the `T` type parameter on `Store<T>` has had no
bounds on it. This was intended for maximal flexibility in terms of what
embedders place within a `Store<T>` and I've personally advocated that
we need to keep it this way. In the development of the WASIp3 work,
however, I've at least personally reached the conclusion that this is no
longer tenable and proceeding will require adding a `'static` bound to
data within a store.

Wasmtime today [already] carries unsafe `transmute`s to work around this
lack of `'static` bound, and while the number of `unsafe` parts is
relatively small right now we're still fundamentally lying to the
compiler about lifetime bounds internally. With the WASIp3 async work
this degree of "lying" has become even worse. Joel has written up some
examples [on Zulip] about how the Rust compiler is requiring `'static`
bounds in surprising ways. These patterns are cropping up quite
frequently in the WASIp3 work and it's becoming particularly onerous
maintaining all of the `unsafe` and ensuring that everything is in sync.

In the WASIp3 repository I've additionally [prototyped a change] which
would additionally practically require `T: 'static` in more locations.
This change is one I plan on landing in Wasmtime in the near future and
while its main motivations are for enabling WASIp3 work it is also a
much nicer system than what we have today, in my opinion.

Overall the cost of not having `T: 'static` on `Store<T>` is effectively
becoming quite costly, in particular with respect to WASIp3 work. This
is coupled with all known embedders already using `T: 'static` data
within a `Store<T>` so the expectation of the impact of this change is
not large. The main downside of this change as a result is that when and
where to place `'static` bounds is sort of a game of whack-a-mole with
the compiler. For example I changed `Store<T>` to require `'static`
here, but the rest of the change is basically "hit compile until rustc
says it's ok". There's not necessarily a huge amount of rhyme-or-reason
to where `'static` bounds crop up, which can be surprising or difficult
to work with for users.

In the end I feel that this change is necessary and one we can't shy
away from. If problems crop up we'll need to figure out how to thread
that needle at that time, but I'm coming around to thinking that
`T: 'static` is just a fundamental constraint we'll have to take on at
this time. Maybe a future version of Rust that fixes some of Joel's
examples (if they can be fixed, we're not sure of that) we could
consider relaxing this but that's left for future work.

[already]: https://github.com/bytecodealliance/wasmtime/blob/35053d6d8d1a5d4692cf636cba0c920b4a79a44b/crates/wasmtime/src/runtime/store.rs#L602-L611
[on Zulip]: https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/.22type.20may.20not.20live.20long.20enough.22.20for.20generic.20closure/near/473862072
[prototyped a change]: bytecodealliance/wasip3-prototyping#158
@alexcrichton alexcrichton mentioned this pull request May 9, 2025
Merged
Merged via the queue into bytecodealliance:main with commit e7bbbd0 May 9, 2025
155 checks passed
github-merge-queue Bot pushed a commit to bytecodealliance/wasmtime that referenced this pull request May 13, 2025
@alexcrichton
Add T: 'static to Store<T> (#10760)
f81c0dc 
* Add `T: 'static` to `Store<T>

Since the beginning the `T` type parameter on `Store<T>` has had no
bounds on it. This was intended for maximal flexibility in terms of what
embedders place within a `Store<T>` and I've personally advocated that
we need to keep it this way. In the development of the WASIp3 work,
however, I've at least personally reached the conclusion that this is no
longer tenable and proceeding will require adding a `'static` bound to
data within a store.

Wasmtime today [already] carries unsafe `transmute`s to work around this
lack of `'static` bound, and while the number of `unsafe` parts is
relatively small right now we're still fundamentally lying to the
compiler about lifetime bounds internally. With the WASIp3 async work
this degree of "lying" has become even worse. Joel has written up some
examples [on Zulip] about how the Rust compiler is requiring `'static`
bounds in surprising ways. These patterns are cropping up quite
frequently in the WASIp3 work and it's becoming particularly onerous
maintaining all of the `unsafe` and ensuring that everything is in sync.

In the WASIp3 repository I've additionally [prototyped a change] which
would additionally practically require `T: 'static` in more locations.
This change is one I plan on landing in Wasmtime in the near future and
while its main motivations are for enabling WASIp3 work it is also a
much nicer system than what we have today, in my opinion.

Overall the cost of not having `T: 'static` on `Store<T>` is effectively
becoming quite costly, in particular with respect to WASIp3 work. This
is coupled with all known embedders already using `T: 'static` data
within a `Store<T>` so the expectation of the impact of this change is
not large. The main downside of this change as a result is that when and
where to place `'static` bounds is sort of a game of whack-a-mole with
the compiler. For example I changed `Store<T>` to require `'static`
here, but the rest of the change is basically "hit compile until rustc
says it's ok". There's not necessarily a huge amount of rhyme-or-reason
to where `'static` bounds crop up, which can be surprising or difficult
to work with for users.

In the end I feel that this change is necessary and one we can't shy
away from. If problems crop up we'll need to figure out how to thread
that needle at that time, but I'm coming around to thinking that
`T: 'static` is just a fundamental constraint we'll have to take on at
this time. Maybe a future version of Rust that fixes some of Joel's
examples (if they can be fixed, we're not sure of that) we could
consider relaxing this but that's left for future work.

[already]: https://github.com/bytecodealliance/wasmtime/blob/35053d6d8d1a5d4692cf636cba0c920b4a79a44b/crates/wasmtime/src/runtime/store.rs#L602-L611
[on Zulip]: https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/.22type.20may.20not.20live.20long.20enough.22.20for.20generic.20closure/near/473862072
[prototyped a change]: bytecodealliance/wasip3-prototyping#158

* Remove a no-longer-necessary `unsafe` block

* Update test expectations

* Fix gc-disabled builds
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@dicej dicej dicej approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alexcrichton @dicej