Stream: git-wasmtime

Topic: wasmtime / PR #4041 Reduce contention on the global modul...

Wasmtime GitHub notifications bot (Apr 15 2022 at 21:24):

alexcrichton opened PR #4041 from less-rwlock to main:

This commit intendes to close #4025 by reducing contention on the global
rwlock Wasmtime has for module information during instantiation and
dropping a store. Currently registration of a module into this global
map happens during instantiation, but this can be a hot path as
embeddings may want to, in parallel, instantiate modules.

Instead this switches to a strategy of inserting into the global module
map when a Module is created and then removing it from the map when
the Module is dropped. Registration in a Store now preserves the
entire Module within the store as opposed to trying to only save it
piecemeal. In reality the only piece that wasn't saved within a store
was the TypeTables which was pretty inconsequential for core wasm
modules anyway.

This means that instantiation should now clone a singluar Arc into a
Store per Module (previously it cloned two) with zero managemnt on
the global rwlock as that happened at Module creation time.
Additionally dropping a Store again involves zero rwlock management
and only a single Arc drop per-instantiated module (previously it was
two).

In the process of doing this I also went ahead and removed the
Module::new_with_name API. This has been difficult to support
historically with various variations on the internals of ModuleInner
because it involves mutating a Module after it's been created. My hope
is that this API is pretty rarely used and/or isn't super important, so
it's ok to remove.

Finally this change removes some internal Arc layerings that are no
longer necessary, attempting to use either T or &T where possible
without dealing with the overhead of an Arc.

Closes #4025

<!--

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 15 2022 at 21:26):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 15 2022 at 21:26):

alexcrichton created PR review comment:

I ideally wanted to remove this Arc as well but the GlobalModuleRegistry needed to hang on to something and the Module isn't constructed yet (and Module needs the dtor to unregister), so I went ahead and left this in an Arc but at this Module layer instead of the previous layer.

Wasmtime GitHub notifications bot (Apr 15 2022 at 22:00):

alexcrichton updated PR #4041 from less-rwlock to main.

Wasmtime GitHub notifications bot (Apr 18 2022 at 20:06):

alexcrichton updated PR #4041 from less-rwlock to main.

Wasmtime GitHub notifications bot (Apr 19 2022 at 14:31):

alexcrichton requested fitzgen for a review on PR #4041.

Wasmtime GitHub notifications bot (Apr 19 2022 at 16:08):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 19 2022 at 16:08):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 19 2022 at 16:08):

fitzgen created PR review comment:

Why switch modules_with_code from a BTreeMap to a sorted Vec? This is introducing O(n) runtime to registering new modules. Is the binary search in the sorted Vec really that much faster that we are willing to take the hit on O(n) module registration?

Wasmtime GitHub notifications bot (Apr 19 2022 at 17:01):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 19 2022 at 17:01):

alexcrichton created PR review comment:

This was a drive-by cleanup I figured I'd do while I was changing things. When thinking again about this I figured that most stores would only ever have a small handful of modules so the overhead of BTreeMap probably wasn't worth it. If you'd prefer though I can add the map back in

Wasmtime GitHub notifications bot (Apr 19 2022 at 17:45):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 19 2022 at 17:45):

fitzgen created PR review comment:

Does the BTreeMap actually have overhead for a small number of modules? If they all fit in a single node, then I think there wouldn't be any overhead at all?

Without profiling evidence to the contrary, I'd lean towards keeping the BTreeMap and avoiding turning registration from O(log n) to O(n). Especially since, with the component model, we will expect smaller but more modules to become the norm.

Wasmtime GitHub notifications bot (Apr 19 2022 at 17:46):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 19 2022 at 17:46):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 19 2022 at 19:34):

alexcrichton updated PR #4041 from less-rwlock to main.

Wasmtime GitHub notifications bot (Apr 19 2022 at 20:13):

alexcrichton merged PR #4041.

Last updated: Nov 22 2024 at 17:03 UTC