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

feat(compiler)!: Deduplicate imports #2233

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

spotandjake
Copy link
Member

@spotandjake spotandjake commented Jan 14, 2025

This enables the binaryen pass to de-duplicate imports, if were against using the binaryen pass I think we can also do this manually in compcore and just filter through the imports in compile_imports but we'll have to manually handle re-patching the import identifiers through the program.

This makes developing for runtimes such as wasmtime a little nicer.

This also adds the assertWasmSnapshot which let's us test wat snapshots for optimizations that occur after, as the regular mashtree snapshots won't catch these, we can get away with a simple print("test") here because multiple places in the runtime import fd_write.

Closes: #2152

Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

I think that you're getting a stack overflow because the snapshot is massive. As of the new object files PR, we don't do snapshots of wat anymore because they're a lot bigger (and thus impossible to really diff/read). The way to test this is programmatically. An example of a test that looks at wasm exports (and you can do the same for imports) is

expect.list(Option.get(export_sections)).toContainEqual((
.

@spotandjake spotandjake force-pushed the spotandjake-dedupe-imports branch from 753c558 to ac65ee8 Compare January 19, 2025 23:53
@ospencer
Copy link
Member

I would lean towards trying to prevent the issue rather than rely on the Binaryen pass. I think you're right that this would be annoying to resolve during codegen, but I don't think this is bad to fix as a part of linking at all, because it's already keeping track of names to rewrite. You could dedupe the imports and just keep the needed name in the resolver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate imports
2 participants