RemoveUnusedModuleElements: Redirect never-called table entries to a shared stub#8887
RemoveUnusedModuleElements: Redirect never-called table entries to a shared stub#8887JPL11 wants to merge 1 commit into
Conversation
…shared stub Functions in element segments that are referenced but not used can never be called successfully: any call_indirect reaching their slot traps on the signature check. Previously we kept each such function with its body emptied to an unreachable, one per function (deduplicatable to one per signature). Instead, point all such entries at a single shared stub function with the simplest signature and an unreachable body, as suggested by kripken in WebAssembly#3029: a call reaching the slot still traps either way (on the signature check, or in the stub), and the redirected functions can then be removed entirely. All entries remain ref.funcs, so segments keep the compact MVP encoding. This is done only when the segment's table is neither imported nor exported (an outside view could observe the changed entries) and when GC is disabled (a table.get plus ref.test could observe the changed type of an entry; emptying a function in place preserves its type, redirecting does not). After redirecting we rescan the module for remaining ref.funcs, since the redirect may have removed the last reference to a function, and remove the ones no longer referred to.
JPL11
requested review from
Copilot and
stevenfontanella
and removed request for
a team
| if (!module->features.hasGC()) { | ||
| std::unordered_set<Name> exportedTables; | ||
| for (auto& ex : module->exports) { | ||
| if (ex->kind == ExternalKind::Table) { |
There was a problem hiding this comment.
I was hoping tableInfoMap would make all these checks trivial - it has hasSet to check for table.sets both inside and outside the module. However, we would need to add hasGet there, for this optimization. I'm not sure if that would be useful enough, and in particular it means more scanning work.
| auto stubHeapType = HeapType(Signature(Type::none, Type::none)); | ||
| auto stubRefType = Type(stubHeapType, NonNullable); | ||
| Name stubName; | ||
| for (auto& segment : module->elementSegments) { |
There was a problem hiding this comment.
I'm worried about this extra scanning work, given the benefit here may be very small.
Maybe it would be good to measure the benefit of this before doing any more work here. Do you have some codebase in mind where you were expecting to see a benefit, or did you just see the open issue?
|
Honest answer: I picked this up from the open issue, I did not have a specific codebase in mind. So I took your suggestion and measured before doing more work. Setup: this branch vs main, both run with
The first two rows are the general story: Emscripten's normal ABI puts the indirect function table on the module boundary, so the precondition this needs (table neither imported nor exported) never holds on typical output. Standalone mode should be the exception, per your old comment on the issue, but I could not find a published standalone corpus to check, and I would expect the same result there since wasm-ld only emits address taken functions into the table to begin with. The last row is the best case I could construct: even with the table made private, sql.js has a single uncallable function left after the rest of I think that settles this PR: the benefit does not justify the complexity, so I would close it. For the record, in case this is ever revived: the extra scanning is avoidable, since the analyzer could track during its existing walk whether a function reference came from code or only from segments (removing the need for any rescan), and a For #3029 itself, I think the honest resolution is that it was solved by evolution rather than by a missing optimization. The core ask from 2020, dropping table entries whose signature matches no call_indirect, has been in the pass for years via the per type liveness (typeFuncs/typeElems). What is left is per entry residue that has now been measured twice at single digit bytes. If you agree, it may be worth closing the issue or at least removing the good first bug label, so the next person does not walk the same path. Happy to write that summary on the issue. |
|
Follow up: I found a standalone corpus after all, so the gap in my last comment is covered now. WASI builds are exactly the standalone case from your old comment: their tables are private, neither imported nor exported.
Same setup as before ( So even where the preconditions hold, with thousands of table entries, real toolchain output has no uncallable entries. In hindsight the structural reason is simple: wasm-ld only emits address taken functions into the table, and in real C and Rust code, every function whose address is taken has its signature called indirectly somewhere. Between this and the Emscripten numbers above, I think the premise of the issue is settled empirically on both ABIs. I will write the summary on #3029 if you want to close it. |
|
Thanks for the data, interesting. Ok, let's close this. I'll update the other issue, feel free to add more too. |
Implements the shared stub idea from #3029 (replacing the approach from the closed #8883).
Functions in element segments that are referenced but not used can never be called successfully: any call_indirect reaching their slot traps on the signature check. Previously each such function was kept with its body emptied to an unreachable. This PR points all such entries at a single shared stub function with the simplest signature and an unreachable body. A call reaching the slot still traps either way (on the signature check, or in the stub if the caller happens to have the stub's type), and the redirected functions are then removed entirely. Since every entry remains a ref.func, segments keep the compact MVP index encoding, avoiding the size regression that nulling entries would cause (discussed on the issue).
Conditions:
After redirecting, the pass rescans the module for remaining ref.funcs, since a redirect may have removed the last reference to a function, and removes the ones no longer referred to. A function that is also referenced from live code keeps its (emptied) definition, and its entry is still redirected.
Tests: new lit files for the redirect, the code-reference case, the imported/exported table cases, and the GC gate. Existing 948 lit tests pass unchanged (the existing closed-world tests for this pass enable GC, where this optimization is gated off). Differential execution on the trap slots shows identical results: a caller with a different signature still traps on the signature check against the stub.
The element segment packing question from the issue (dropping runs of identical entries) is left as a possible follow-up; this change is independent of it.