Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm-split] Run RemoveUnusedElements on secondary modules #6945

Merged
merged 10 commits into from
Sep 17, 2024

Conversation

tlively
Copy link
Member

@tlively tlively commented Sep 16, 2024

Rather than analyze what module elements from the primary module a
secondary module will need, the splitting logic conservatively imports
all module elements from the primary module into the secondary module.
Run RemoveUnusedElements on the secondary module to remove any of these
imports that happen to be unnecessary. Leave a TODO mentioning the
possibility of being more selective about which module elements get
exported to reduce code size in the primary module, too.

Wasm-split generally assumes that calls to secondary functions made
before the secondary module has been loaded and instantiated should go
to imported placeholder functions that can be responsible for loading
the secondary module and forwarding the call to the loaded function.
That scheme makes the loading entirely transparent from the
application's point of view, which is not always a good thing. Other
schemes would make it impossible for a secondary function to be called
before the secondary module has been explicitly loaded, in which case
the placeholder functions would never be called. To improve code size
and simplify instantiation under these schemes, add a new
`--no-placeholders` option that skips adding imported placeholder
functions.
Add a mode that splits a module into arbitrarily many parts based on a
simple manifest file. This is currently implemented by splitting out one
module at a time in a loop, but this could change in the future if
splitting out all the modules at once would improve the quality of the
output.
Rather than analyze what module elements from the primary module a
secondary module will need, the splitting logic conservatively imports
all module elements from the primary module into the secondary module.
Run RemoveUnusedElements on the secondary module to remove any of these
imports that happen to be unnecessary. Leave a TODO mentioning the
possibility of being more selective about which module elements get
exported to reduce code size in the primary module, too.
@@ -334,6 +335,7 @@ struct ModuleSplitter {
exportImportCalledPrimaryFunctions();
setupTablePatching();
shareImportableItems();
removeUnusedElements();
Copy link
Member

@kripken kripken Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this ran on the primary then it would run after each of the N splits in MultiSplit mode, which would be slow - but it looks like you only run it on the secondary? If so maybe rename this to reflect that?

I guess it's not possible the primary can also benefit from this? (assuming the module is already optimized)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, everything besides functions is exported from the primary module, so there would be no removable module elements.

I'll clarify the method name.

@@ -1,23 +1,34 @@
;; RUN: wasm-split %s --keep-funcs=foo,bar --export-prefix='%' -o1 %t.1.wasm -o2 %t.2.wasm
;; RUN: wasm-split %s --keep-funcs=foo,bar --export-prefix='%' -o1 %t.1.wasm -o2 %t.2.wasm --no-placeholders
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to simplify the output a little bit. It's one less new import in the primary module. We newly export $baz to make sure it is kept alive in the secondary module via a use in the primary module.

@@ -16,7 +16,7 @@

(module
(func $foo
(nop)
(call $bar)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent $bar from being removed for being unused in the secondary module.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % questions

Base automatically changed from wasm-split-multi-split to main September 16, 2024 22:57
@tlively tlively enabled auto-merge (squash) September 17, 2024 00:10
@tlively tlively merged commit 34ad6a7 into main Sep 17, 2024
13 checks passed
@tlively tlively deleted the wasm-split-remove-unused branch September 17, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants