Stream: git-wasmtime

Topic: wasmtime / PR #2820 Speed up registration of stack maps

Wasmtime GitHub notifications bot (Apr 08 2021 at 17:57):

alexcrichton opened PR #2820 from faster-stack-maps to main:

This commit fixes a perf issue I was seeing in some local benchmarking
where registration of stack maps took a nontrivial amount of time for a
module that didn't even use stack maps! The fix here is to largely just
do the same thing as #2811 which is to use the in-memory data structures
of a CompiledModule rather than rebuilding different data structures
when a module is registered with a Store.

The StackMapRegistry type now takes a lookup object for a range of
pcs, simplifying registration. Registration additionally doesn't even
happen if a module is pre-determined to not have any stack maps inside
of it. This trait object encapsulates all the logic that was previously
used in the rebuilt data structures and also leverages conveniences like
CompiledModule::func_by_pc added recently as well.

With this all combined it means that modules which don't have stack maps
now skip this registration step entirely and modules with stack maps
should do much less work during registration as well.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 08 2021 at 17:57):

alexcrichton requested fitzgen for a review on PR #2820.

Wasmtime GitHub notifications bot (Apr 08 2021 at 18:35):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 19:52):

peterhuene created PR Review Comment:

Question: why the trait over using a method on CompiledModule (similar to func_by_pc and func_info)?

Wasmtime GitHub notifications bot (Apr 08 2021 at 19:52):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 19:53):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:05):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:05):

alexcrichton created PR Review Comment:

Only because the CompiledModule lives in the wasmtime_jit crate which this wasmtime_runtime crate doesn't have access to. Otherwise though there's definitely no need for dynamic trait dispatch here.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:06):

alexcrichton updated PR #2820 from faster-stack-maps to main.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:20):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:20):

peterhuene created PR Review Comment:

Makes sense. I guess the alternative would be to use a "lookup" trait that the runtime interacts with (it would just need the lookup_stack_map method), freeing the registry implementation to live in the wasmtime crate, but that's just trading dynamic-dispatch during module instantiation to dynamic-dispatch at externref gc.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:20):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:36):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:36):

peterhuene created PR Review Comment:

If we did move the registry implementation to wasmtime, I think this data structure could be a generic one replacing StoreFrameInfo too, basically just a "which CompiledModule should I be talking to given this pc?" which responds with an answer that we then use to lookup the appropriate information (gimmie frame info, gimmie trap info, gimmie stack maps, etc.)

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:41):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:53):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:54):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 08 2021 at 20:56):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 08 2021 at 21:06):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 21:06):

peterhuene created PR Review Comment:

In fact, given externref gc being a relatively expensive (and hopefully very infrequent) operation and the cost of doing trait object dispatch per frame would be negligible, I would argue VMContext should have *const dyn StackMapLookup rather than *mut StackMapRegistry.

This frees us to remove StackMapRegistry and StoreFrameInfo in favor of a single CompiledModuleRegistry per store which impls the runtime's StackMapLookup and can lookup frame and trap information as well.

Thoughts on this approach? Perhaps for another PR?

Wasmtime GitHub notifications bot (Apr 08 2021 at 21:08):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 08 2021 at 22:13):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 22:13):

alexcrichton created PR Review Comment:

Hm good points! I'll look to add this to this PR, seems reasonable to do here!

Wasmtime GitHub notifications bot (Apr 15 2021 at 14:01):

alexcrichton closed without merge PR #2820.

Last updated: Nov 22 2024 at 16:03 UTC