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

Premature optimization of modules with side effects #1903

Open
spotandjake opened this issue Aug 9, 2023 · 1 comment · May be fixed by #2104
Open

Premature optimization of modules with side effects #1903

spotandjake opened this issue Aug 9, 2023 · 1 comment · May be fixed by #2104
Assignees
Labels

Comments

@spotandjake
Copy link
Member

If you have module b with the contents:

module B
print("b")

and module a with the contents:

module A
print("b")
include "./b"
print("c")

when you call grain a.gr instead of getting the expected

a
b
c

in the console you get

b
c

It seems like we might be prematurely optimizing modules because when I add an export and use that export the program does whats expected. I think we need to consider side effects in tree shaking.

@phated phated moved this to In Progress in Grain v0.6.0 Aug 20, 2023
@phated phated moved this from In Progress to Todo in Grain v0.6.0 Aug 20, 2023
@spotandjake
Copy link
Member Author

spotandjake commented Jan 15, 2024

I was looking into this a bit the other day, I think this is a problem in the linker, where we don't currently link with a module if there is no wasm import, and as we are not using anything from the module we dont have any imports to it.

I think the best way to fix this would be to read the imports from the cmi, or if we want an approach that makes the intermediate wasm slightly more correct, we could import the _start function and call it in the main modules _start function, though its important to keep in mind if we use the second approach that we cant just reoutput the _start section we would need to deduplicate and properly order the _start calls. I am a fan of method two as it works better with dynamic linking which we no longer support but its cool to leave the option open.

@phated phated removed the status in Grain v0.6.0 Feb 1, 2024
@phated phated removed this from Grain v0.6.0 Feb 15, 2024
@spotandjake spotandjake linked a pull request May 4, 2024 that will close this issue
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 a pull request may close this issue.

3 participants