[wasm-split] Split active segments (part 1) - #8840
Conversation
This removes the assumption that all segments should be pinned to the primary module. This first checks memory and table usage, and if a memory/table is exclusively used in a secondary module, moves active data / elem segments that reference the table to that secondary module. We check whether a segment can trap, and if so, we pin it to the primary because they should be evaluated at the primary instantiation time. When a memory/table is used in multiple modules, it is suboptimal to just scan all segments of that memory/table and mark them as used in those modules, because as in the case of #????, this can generate unnecessary exports. For example, if table $t is used both in the primary and the secondary, and elem $e is like ```wast (elem $e (table $t) (i32.const 0) ... (global.get $g)) ``` because $e will stay in the primary module, the secondary module is not going to see $g, so it should NOT be exported and imported. This PR figures out a single owner module for each memory/table, and marks segments as "used" there. This does not currently reduces the size of primary modules of acx_gallery and essentials because every table is exported, so there is no tables exclusively used by a secondary. This may change when we remove internal exports.
| if (segment->offset && | ||
| EffectAnalyzer(config.passOptions, primary, segment->offset) | ||
| .hasUnremovableSideEffects()) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I don't think it's possible for an i32 constant expression to have side effects, since the only source of side effects is struct allocations with null descriptors. Maybe we can turn this into an assertion?
There was a problem hiding this comment.
You're right. I just removed the whole if. Maybe not even worth an assertion.
(In case you feel we need an assertion after all, let me know)
| } else { | ||
| segmentSize = segment->data.size(); | ||
| auto* table = primary.getTable(segment->table); | ||
| parentSize = table->initial * Table::kPageSize; |
There was a problem hiding this comment.
Table::kPageSize should just be 1 (I guess we define it to make some binary reader or writer code more general?) so I don't think we need to use it here.
| using AddressType = Address::address64_t; | ||
| AddressType maxWritten; |
There was a problem hiding this comment.
Is there any advantage to this over just using uint64_t?
There was a problem hiding this comment.
They are the same thing:
Line 56 in c043408
And the reason I used this was just I copied the code from here:
binaryen/src/passes/RemoveUnusedModuleElements.cpp
Lines 889 to 890 in c043408
Either should be fine. Changed to uint64_t: bff8917
| if (!inPrimary && usingSecondaries.empty()) { | ||
| dataSegmentsToRemove.push_back(dataSegment->name); |
There was a problem hiding this comment.
I think this pattern of having compute and check all the using secondaries could also be simplified if we computed the single owning module for every item as suggested in #8832 (comment).
There was a problem hiding this comment.
Can I do that as a followup?
2 participants
This removes the assumption that all segments should be pinned to the primary module. This first checks memory and table usage, and if a memory/table is exclusively used in a secondary module, moves active data / elem segments that reference the table to that secondary module.
We check whether a segment can trap, and if so, we pin it to the primary because they should be evaluated at the primary instantiation time.
When a memory/table is used in multiple modules, it is suboptimal to just scan all segments of that memory/table and mark them as used in those modules, because as in the case of #8832, this can generate unnecessary exports. For example, if table $t is used both in the primary and the secondary, and elem $e is like
because $e will stay in the primary module, the secondary module is not going to see $g, so it should NOT be exported and imported. This PR figures out a single owner module for each memory/table, and marks segments as "used" there.
This does not currently reduces the size of primary modules of acx_gallery and essentials because every table is exported, so there is no tables exclusively used by a secondary. This may change when we remove internal exports.