Opportunisticly emit compact imports#8926
Conversation
tlively
changed the title
| if (const auto* fa = std::get_if<Function*>(&a)) { | ||
| auto* fb = std::get<Function*>(b); | ||
| return (*fa)->type.isExact() == fb->type.isExact() && | ||
| (*fa)->type.getHeapType() == fb->type.getHeapType(); |
There was a problem hiding this comment.
Why isn't this just (*fa)->type == fb->type?
There was a problem hiding this comment.
No reason, that would be a good simplification.
| return (*ma)->initial == mb->initial && (*ma)->max == mb->max && | ||
| (*ma)->hasMax() == mb->hasMax() && (*ma)->shared == mb->shared && | ||
| (*ma)->is64() == mb->is64() && | ||
| (*ma)->pageSizeLog2 == mb->pageSizeLog2; |
There was a problem hiding this comment.
We have MemoryUtils::isSubType - we can check a <= b && b <= a here? (maybe adding a helper for it in MemoryUtils?)
There was a problem hiding this comment.
Sure, or if that method isn't appropriate due to the runtime interaction Steven mentioned, I can create another helper.
| auto* tb = std::get<Table*>(b); | ||
| return (*ta)->type == tb->type && (*ta)->initial == tb->initial && | ||
| (*ta)->max == tb->max && (*ta)->hasMax() == tb->hasMax() && | ||
| (*ta)->is64() == tb->is64(); |
There was a problem hiding this comment.
We have RuntimeTable::isSubType, which we could use like above? Though perhaps first with moving isSubType to TableUtils?)
There was a problem hiding this comment.
(in these two comments I am trying to avoid duplicating this logic across the codebase, which would make later refactors harder/error prone)
There was a problem hiding this comment.
FWIW isSubType is in RuntimeTable because it's not possible to statically determine whether one table is a subtype of another. table.grow will affect the subtyping relationship and it would be wrong to try to compare two table definitions for subtyping (this is a bug we previously had in the interpreter).
This is checking if two table definitions are equivalent statically, so I think we could keep this logic separate or introduce a new helper function for it.
There was a problem hiding this comment.
Oh, good point. Perhaps we can share some of the logic between those, though?
| ;; Build to a binary wasm. | ||
| ;; | ||
| ;; RUN: wasm-opt %s -o %t.wasm -q -all | ||
| ;; RUN: wasm-opt %s -o %t.wasm -q -all --disable-compact-imports |
There was a problem hiding this comment.
-all makes the output Wasm contain compact imports, but the V8 run later in this file does not enable compact imports. We could alternatively have fixed this by enabled compact imports in V8, which I suppose would have the benefit of showing that V8 parses them correctly. I'll change this.
There was a problem hiding this comment.
is --wasm-staging not enough, btw, for v8 to support compact imports? That would be more general if so.
There was a problem hiding this comment.
Turns out --wasm-staging is not enough. I'll specifically enable --wasm-compact-imports.
| wasm_bytes = self.get_binary(wat, ['--enable-compact-imports']) | ||
| self.assertIn(b'\x03env\x00\x7e', wasm_bytes) | ||
| self.assertIn(b'\x04math\x00\x7f', wasm_bytes) | ||
| self.assertIn(b'\x06single\x02m1', wasm_bytes) |
There was a problem hiding this comment.
Maybe it would be good to add a code size test here, something like 1,000 procedurally-generated imports with the same module, and seeing how much smaller the binary size is with the feature enabled?
There was a problem hiding this comment.
Sure, I guess that would catch accidental regressions.
| template<class... Ts> struct Overloaded : Ts... { | ||
| using Ts::operator()...; | ||
| }; | ||
| template<class... Ts> Overloaded(Ts...) -> Overloaded<Ts...>; | ||
|
|
There was a problem hiding this comment.
Ah, thanks! I thought I had seen it before.
| ModuleUtils::iterImportedFunctions( | ||
| *wasm, [&](Function* func) { imports.push_back(func); }); | ||
| ModuleUtils::iterImportedGlobals( | ||
| *wasm, [&](Global* global) { imports.push_back(global); }); | ||
| ModuleUtils::iterImportedTags(*wasm, | ||
| [&](Tag* tag) { imports.push_back(tag); }); | ||
| ModuleUtils::iterImportedMemories( | ||
| *wasm, [&](Memory* memory) { imports.push_back(memory); }); | ||
| ModuleUtils::iterImportedTables( | ||
| *wasm, [&](Table* table) { imports.push_back(table); }); |
There was a problem hiding this comment.
This can be simplified to
ModuleUtils::iterImports(*wasm, [&](ImportItem import) { imports.push_back(import); }3 participants
In the binary writer when compact imports are enabled, opportunistically look for pairs of adjacent imports that share both their module names and import types, or alternatively just their module names. When a pair of matching imports is found, create a run of compact imports that includes as many subsequent imports as possible. Adding a pass that will reorder imports to purposefully put similar imports together is left as future work.