wasmtime icon indicating copy to clipboard operation
wasmtime copied to clipboard

Allow multiple registration of same image in the GlobalRegistry

Open Milek7 opened this issue 6 months ago • 5 comments

With Module::deserialize_raw it is possible to create multiple CodeMemory objects that refer to the same underlying image. This is useful when embedder uses multiple Engine objects with different config. Currently this fails on assertion when trying to register same region of code second time in the global_code registry.

Global code registry stores Arc<CodeMemory>, but only needs to access trap data section in the image. Storing list of these in code registry would be silly, because on PC lookup one item would need to be picked arbitrarily, possibly different than the one which is currently executing, only to access trap_data field which must be identical for all items.

This commit modifies global registry to only store raw pointers to the trap_data section along with an usage count. Unregistration is moved into Drop guard to ensure that underlying data is not freed before unregistration is manually called.

Milek7 avatar May 30 '25 19:05 Milek7

This is useful when embedder uses multiple Engine objects with different config.

If the engines have different configs, then the same compiled artifact will not be compatible with all engines, only the one using the config that it was compiled with.

fitzgen avatar May 30 '25 20:05 fitzgen

(And if the engines all have the same config, then you might as well just use one engine.)

fitzgen avatar May 30 '25 20:05 fitzgen

I think it's not nice if it crashes on assert just because you used same image more than once. There's no such limitation for modules loaded from normal file. (and to be precise there are config settings that are not dependent on the artifact, like stack limits, memory allocators and few others)

Milek7 avatar May 30 '25 20:05 Milek7

One comment on this I've got is that we've generally been trying to reduce unsafe usage in Wasmtime wherever possible, so if this is added it'd be great to do it without increasing the amount of unsafe. Additionally can you ensure there's a test for this? Otherwise it's pretty likely functionality is lost in a future refactoring.

alexcrichton avatar Jun 04 '25 22:06 alexcrichton

I think avoiding unsafe would require storing registry as BTreeMap<usize, Vec<Arc<CodeMemory>>>, would that be preferable?

Milek7 avatar Jun 06 '25 19:06 Milek7