[wasm-split] Precompute ownership info (NFC) - #8986
Conversation
Given a module element name, many parts of the code queries for its owning modules (where the module element has to be placed) or secondary modules using that module element. This adds `OwnershipTracker`, which precomputes and manages that information. All calls to `getOwner` or `getUsingSecondaries` that required computations iterating on all secondary modules which can be as many as thousands, has been replaced with a call that simply returns prcomputed information. For the Jul 2026 version of the applications received from the Dart team, this reduces the running time of wasm-split by 17% for acx_gallery (30s -> 25s) and by 33% for essentials (230s -> 153s). Suggested in #8832 (comment).
Previously we removed module elements one by one within a loop. But because `Module` stores a module element in both a map and a vector, removing a single module element using `removeModuleElement` is O(N), because it needs to shift all vector elements after it: https://github.com/WebAssembly/binaryen/blob/302396a676433152a32375a81d71e74687c97a1b/src/wasm/wasm.cpp#L1970-L1979 This removes module elements in bulk using `removeModuleElements`, which does the shifting only once. https://github.com/WebAssembly/binaryen/blob/302396a676433152a32375a81d71e74687c97a1b/src/wasm/wasm.cpp#L2004-L2018 Combining with #8986, acx_gallery's running time improved by 50.3% (30s -> 15s), and essentials by 60.8% (230s -> 90s). (for Jul 2026 version) I guess the main reason for the running time increase in #8441 was this O(N) `removeModuleElement` called within a loop after all.
| std::unordered_map<Name, ItemInfo> dataSegments; | ||
| std::unordered_map<Name, ItemInfo> elementSegments; | ||
|
|
||
| std::unordered_map<UsedNames*, Module*> usedToSecondary; |
There was a problem hiding this comment.
Can we remove the need for this map by identifying entries in UsedNames by secondary module index rather than by UsedName*?
There was a problem hiding this comment.
Tried it, but not sure this is what you wanted: ba70880
This removes usedToSecondary and figures out which secondary we are referring to by a pointer arithmetic. Did you have something else in mind by "by secondary module index rather than by UsedName*?
| // used by the primary module or multiple secondary modules, the primary | ||
| // module is the owner. | ||
| auto [it, inserted] = (this->*mapField).insert({name, ItemInfo{owner, {}}}); | ||
| Module* mod = usedToSecondary[owner]; |
There was a problem hiding this comment.
We can use the relatively new find_or_null utility from src/support/utilities.h here.
| if (std::find(vec.begin(), vec.end(), mod) == vec.end()) { | ||
| vec.push_back(mod); | ||
| } |
There was a problem hiding this comment.
Since we need to avoid adding the same secondary module to usingSecondaries twice, should usingSecondaries be a set?
There was a problem hiding this comment.
for (auto* secondary :
tracker.getUsingSecondaries(memory->name, tracker.memories)) {
auto* secondaryMemory =
ModuleUtils::copyMemory(memory.get(), *secondary);
makeImportExport(
*memory, *secondaryMemory, "memory", ExternalKind::Memory);
}We iterate on it in shareImportableItems. The iteration order will be determined by pointer values, so they can be different for each run. I didn't think that loop could produce different results for different iteration order, but I thought it was safer not to do that. Maybe it's fine as long as the code doesn't depend on the iteration order?
We also have InsertOrderedSet, which is a set but also preserves the insertion order. I thought it might be overkill.
I tried both and the running time (3-run average) of acx_gallery is:
SmallVector(current): 25.2sSmallSet: 26.3sInsertOrderedSet: 27.1s
| // In the initial building phase, we just directly add to a UsedName struct. | ||
| // After OwnershipTracker is constructed, we all its insert() method to update | ||
| // owner modules and using secondary modules correctly. |
There was a problem hiding this comment.
How do these two phases relate to the general process of splitting described in the comment at the top of the file?
There was a problem hiding this comment.
All of shareImportableItems (and computeUsedNames called from shareImportableItems and construction of OwnershipTracker inside computeUsedNames) correspond to this paragraph:
// 4. Export globals, tags, tables, and memories from the primary module and
// import them in the secondary modules. If possible, move those module
// items instead to the secondary modules.The reason ADD_ITEM does two different things is, before we create OwnershipTracker, we build UsedName for each module, and used.field.insert(val); is done in this phase. After OwnershipTracker is created, we add more constraints. But unlike the straightforward first scanning phase this constraints can add one module item to multiple modules, and we need to keep track of which one should be the owner and such. So in this second phase we use tracker->insert.
Co-authored-by: Thomas Lively <tlively@google.com>
2 participants
Given a module element name, many parts of the code queries for its owning modules (where the module element has to be placed) or secondary modules using that module element. This adds
OwnershipTracker, which precomputes and manages that information. All calls togetOwnerorgetUsingSecondariesthat required computations iterating on all secondary modules which can be as many as thousands, has been replaced with a call that simply returns prcomputed information.For the Jul 2026 version of the applications received from the Dart team, this reduces the running time of wasm-split by 17% for acx_gallery (30s -> 25s) and by 33% for essentials (230s -> 153s).
Suggested in #8832 (comment).