Stream: git-wasmtime

Topic: wasmtime / PR #2811 Refactor store frame information.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 19:25):

peterhuene opened PR #2811 from improve-store-registration to main:

This PR refactors the store frame information to eliminate the copying of
data out from CompiledModule.

It also moves the population of a BTreeMap out of the frame information and
into CompiledModule where it is only ever calculated once rather than at
every new module instantiation into a Store. The map is also lazy-initialized
so the cost of populating the map is incurred only when a trap occurs.

This should help improve instantiation time of modules with a large number of
functions and functions with lots of instructions.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 19:34):

peterhuene requested alexcrichton for a review on PR #2811.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 19:46):

peterhuene submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 19:46):

peterhuene created PR Review Comment:

I just realized this doesn't need to be Arc any more; will fix.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 19:46):

peterhuene edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 19:47):

peterhuene updated PR #2811 from improve-store-registration to main.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 20:07):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 20:07):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 20:07):

alexcrichton created PR Review Comment:

The is_wasm_pc function is somewhat special in that it can primarily get invoked from a signal handler on Unix and Windows. Technically we're doing signal-unsafe things here by grabbing a lock as soon as we enter this function, but it's presumed fine in that we're pretty unlikely to segfault in this function itself. This reminds me, though, could you add some words to this function's documentation indicating that it's called from signal handlers and as a result the contents should be carefully written?

I was originally worried slightly that this call to func would involve assembling the BTreeMap which could involve allocations which could be tricky in a signal handler native-stack wise, but in thinking about it I'm not too worried about that.

Thinking a bit more about this, though, I'm not actually sure if we need the BTreeMap any more. Since func_map() is only ever used for function lookup, we can probably just do a binary search on an array with the pc? I presume that the list of functions is sorted in address order in some array in the compiled artifacts anyway. If we use that I think we can actually remove the need for that whole map entirely?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 20:07):

alexcrichton created PR Review Comment:

I think this can avoid returning an Option? Most of the usage of indexing we have through things like DefinedFuncIndex we have panic if invalid

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 20:46):

peterhuene submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 20:46):

peterhuene created PR Review Comment:

I agree, we probably don't need a BTreeMap here. I'll improve this further.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 21:39):

peterhuene submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 21:39):

peterhuene created PR Review Comment:

I think we can guarantee that finished_functions are in sort-order (they're based on the ELF symbols which we are writing in-order) so we should be able to just utilize that for the search (based on the start vs. the end).

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 23:12):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 23:12):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 23:12):

fitzgen created PR Review Comment:

We could do a with_capacity here.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 23:13):

peterhuene submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 23:13):

peterhuene created PR Review Comment:

Good eye! Thankfully I'm about to push up a change that removes this BTreeMap and just does a binary search on the finished function ranges directly.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 23:38):

peterhuene updated PR #2811 from improve-store-registration to main.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 23:39):

peterhuene requested fitzgen for a review on PR #2811.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2021 at 23:39):

peterhuene requested alexcrichton and fitzgen for a review on PR #2811.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2021 at 00:00):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2021 at 00:00):

fitzgen submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2021 at 00:07):

peterhuene edited PR #2811 from improve-store-registration to main:

This PR refactors the store frame information to eliminate the copying of
data out from CompiledModule.

It also removes the population of a BTreeMap in favor of doing a binary
search on the finished functions in the associated CompiledModule.

This should help improve instantiation time of modules with a large number of
functions and functions with lots of instructions.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2021 at 01:16):

peterhuene merged PR #2811.

Last updated: Dec 23 2024 at 12:05 UTC