alexcrichton opened PR #2319 from remove-trampolines-from-instance
to main
:
This commit refactors where trampolines and signature information is
stored within aStore
, namely moving them from
wasmtime_runtime::Instance
instead toStore
itself. The goal here is
to remove an allocation inside of anInstance
and make them a bit
cheaper to create. Additionally this should open up future possibilities
like not creating duplicate trampolines for signatures already in the
Store
when usingFunc::new
.
alexcrichton updated PR #2319 from remove-trampolines-from-instance
to main
:
This commit refactors where trampolines and signature information is
stored within aStore
, namely moving them from
wasmtime_runtime::Instance
instead toStore
itself. The goal here is
to remove an allocation inside of anInstance
and make them a bit
cheaper to create. Additionally this should open up future possibilities
like not creating duplicate trampolines for signatures already in the
Store
when usingFunc::new
.
pepyakin submitted PR Review.
pepyakin created PR Review Comment:
Since this now stores trampolines, it would be great to update the comment.
alexcrichton updated PR #2319 from remove-trampolines-from-instance
to main
:
This commit refactors where trampolines and signature information is
stored within aStore
, namely moving them from
wasmtime_runtime::Instance
instead toStore
itself. The goal here is
to remove an allocation inside of anInstance
and make them a bit
cheaper to create. Additionally this should open up future possibilities
like not creating duplicate trampolines for signatures already in the
Store
when usingFunc::new
.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Indeed!
fitzgen submitted PR Review.
fitzgen submitted PR Review.
fitzgen created PR Review Comment:
Since this function has to take ownership, we might as well make the caller do the clone (if necessary) so that we can avoid clones when they aren't needed. That is, revert the change from taking
&WasmFuncType
and&ir::Signature
back to takingWasmFuncType
andir::Signature
.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
This is sort of the
Entry
problem where it's problematic that getting anentry
requireswasm.clone()
(should probably just so aget
followed by aninsert
, but also that thenative
isn't always inserted here (although it sometimes is)If we think this is an issue I could change the arguments to
impl Into<Cow<'_, ...>>
, but I was hoping to optimize more for the cache hit use case here rather than the "need to insert case". Do you think I should try to go theCow
route though?
fitzgen submitted PR Review.
fitzgen created PR Review Comment:
If it doesn't require bending over backwards too much, it would be nice.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Hm it turns out that
Into<Cow>
doesn't have as many implementations as I thought it did so I don't think this will work easily. Digging in more though I'm realizing that we're doing a number of unnecessary allocations in a number of places. In general aModule
has no need to store the native signature types (it only needs the wasm signatures).Would you be ok landing this as-is and I can have a follow-up which I think makes all the callers only have
&WasmFuncType
anyway so there's no gain to be had by passing in an owned value?
fitzgen submitted PR Review.
fitzgen created PR Review Comment:
Yeah, for sure
fitzgen merged PR #2319.
Last updated: Dec 23 2024 at 12:05 UTC