Stream: git-wasmtime

Topic: wasmtime / PR #2319 Refactor how signatures/trampolines a...

Wasmtime GitHub notifications bot (Oct 25 2020 at 22:56):

alexcrichton opened PR #2319 from remove-trampolines-from-instance to main:

This commit refactors where trampolines and signature information is
stored within a Store, namely moving them from
wasmtime_runtime::Instance instead to Store itself. The goal here is
to remove an allocation inside of an Instance 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 using Func::new.

Wasmtime GitHub notifications bot (Oct 25 2020 at 22:59):

alexcrichton updated PR #2319 from remove-trampolines-from-instance to main:

This commit refactors where trampolines and signature information is
stored within a Store, namely moving them from
wasmtime_runtime::Instance instead to Store itself. The goal here is
to remove an allocation inside of an Instance 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 using Func::new.

Wasmtime GitHub notifications bot (Oct 30 2020 at 23:09):

pepyakin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 30 2020 at 23:09):

pepyakin created PR Review Comment:

Since this now stores trampolines, it would be great to update the comment.

Wasmtime GitHub notifications bot (Nov 02 2020 at 15:54):

alexcrichton updated PR #2319 from remove-trampolines-from-instance to main:

This commit refactors where trampolines and signature information is
stored within a Store, namely moving them from
wasmtime_runtime::Instance instead to Store itself. The goal here is
to remove an allocation inside of an Instance 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 using Func::new.

Wasmtime GitHub notifications bot (Nov 02 2020 at 15:54):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Nov 02 2020 at 15:54):

alexcrichton created PR Review Comment:

Indeed!

Wasmtime GitHub notifications bot (Nov 03 2020 at 00:33):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 00:34):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 00:34):

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 taking WasmFuncType and ir::Signature.

Wasmtime GitHub notifications bot (Nov 03 2020 at 02:08):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 02:08):

alexcrichton created PR Review Comment:

This is sort of the Entry problem where it's problematic that getting an entry requires wasm.clone() (should probably just so a get followed by an insert, but also that the native 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 the Cow route though?

Wasmtime GitHub notifications bot (Nov 03 2020 at 18:09):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 18:09):

fitzgen created PR Review Comment:

If it doesn't require bending over backwards too much, it would be nice.

Wasmtime GitHub notifications bot (Nov 03 2020 at 20:13):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 20:13):

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 a Module 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?

Wasmtime GitHub notifications bot (Nov 04 2020 at 18:49):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Nov 04 2020 at 18:49):

fitzgen created PR Review Comment:

Yeah, for sure

Wasmtime GitHub notifications bot (Nov 04 2020 at 18:49):

fitzgen merged PR #2319.

Last updated: Nov 22 2024 at 16:03 UTC