Reduce contention on the global module rwlock#4041
Conversation
| types: TypeTables, | ||
| ) -> Result<Self> { | ||
| let module = CompiledModule::from_artifacts( | ||
| let module = Arc::new(CompiledModule::from_artifacts( |
There was a problem hiding this 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.
github-actions
Bot
added
wasmtime:api
Subscribe to Label Actioncc @fitzgen, @peterhuene DetailsThis issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
This commit intendes to close bytecodealliance#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 bytecodealliance#4025
119c403 to
1d7fe0d
Compare
| modules_with_code: BTreeMap<usize, Arc<RegisteredModule>>, | ||
| modules_without_code: Vec<Arc<CompiledModule>>, | ||
| // Sorted by module start address in memory, used for looking up based on a | ||
| // pc of a wasm function. | ||
| modules_with_code: Vec<Module>, |
There was a problem hiding this 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?
There was a problem hiding this 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
There was a problem hiding this 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.
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
r=me with either the BTreeMap reinstated or with profiling evidence that it isn't worth it / binary searching a sorted vec is faster in practice.
|
Sounds good to me, I haven't profiled this so I'll switch back to |
2 participants
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
Moduleis created and then removing it from the map whenthe
Moduleis dropped. Registration in aStorenow preserves theentire
Modulewithin the store as opposed to trying to only save itpiecemeal. In reality the only piece that wasn't saved within a store
was the
TypeTableswhich was pretty inconsequential for core wasmmodules anyway.
This means that instantiation should now clone a singluar
Arcinto aStoreperModule(previously it cloned two) with zero managemnt onthe global rwlock as that happened at
Modulecreation time.Additionally dropping a
Storeagain involves zero rwlock managementand only a single
Arcdrop per-instantiated module (previously it wastwo).
In the process of doing this I also went ahead and removed the
Module::new_with_nameAPI. This has been difficult to supporthistorically with various variations on the internals of
ModuleInnerbecause it involves mutating a
Moduleafter it's been created. My hopeis 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
Arclayerings that are nolonger necessary, attempting to use either
Tor&Twhere possiblewithout dealing with the overhead of an
Arc.Closes #4025