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

Remove static allocation from JS compiler #11773

Closed
sbc100 opened this issue Jul 31, 2020 · 7 comments · Fixed by #11821
Closed

Remove static allocation from JS compiler #11773

sbc100 opened this issue Jul 31, 2020 · 7 comments · Fixed by #11821
Labels

Comments

@sbc100
Copy link
Collaborator

sbc100 commented Jul 31, 2020

We have a function in the JS compiler called makeStaticAlloc that can allocate static data after the linker has run.

This can be a useful feature but it comes with a lot of extra code complexity and downsides.

The biggest problem is that the linker traditionally assumes that it controls data layout, and knows the start and end of the data, stack, and heap regions. With emscripten we have hacks to retroactively update the internal __stack_pointer and inject the stack limit in some cases. It also confuses code that relies on linker-generated symbols such as edata.

The second problem is that any such allocations are not available in standalone mode.

The more of these allocations we can push into native code the fewer JS hacks we will need and the fewer magic binaryen step will be needed. Giving the linker visibility of all these symbols also allows gives it a better chance to do DCE and results in a wasm binary with fewer imports.

For example emscripten_stack_get_base and emscripten_stack_get_end currently call out to JS which is bad for performance and code size and portability.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 31, 2020

I've tried to do this in the past of found some places where it was hard/impossible for me to replace makeStaticAlloc, but I'd like to try again :)

@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 31, 2020

Removing makeStaticAlloc would also remove STATIC_BUMP from the JS codebase, which would simplify a lot of code.

sbc100 pushed a commit that referenced this issue Jul 31, 2020
This is a followup on #11437 which added this API.  Sadly today the
linker doesn't know the final memory layout so until we fix #11773
this extra `init` function is needed.

The alternative, which is to modify binaryen and emscripten to inject
the stack end value post-link is IMHO way to much complexity to justify
the convenience of not having the call this init function.

Once we fix #11773 the init function can become uneeded/no-op.
@gerald-dotcom
Copy link

Out of curiosity, Can this affect Asyncify?

@kripken
Copy link
Member

kripken commented Jul 31, 2020

@sbc100 sounds good - we should probably wait for fastcomp removal first though to avoid extra complexity and work.

I imagine this would require moving a bunch of things from JS into C or .s files, which should be a good improvement for them anyhow.

@gerald-dotcom I don't think asyncify does static allocations (it does dynamic allocations, but those are fine)

sbc100 pushed a commit that referenced this issue Aug 5, 2020
This is a followup on #11437 which added this API.  Sadly today the
linker doesn't know the final memory layout so until we fix #11773
this extra `init` function is needed.

The alternative, which is to modify binaryen and emscripten to inject
the stack end value post-link is IMHO way to much complexity to justify
the convenience of not having the call this init function.

Once we fix #11773 the init function can become uneeded/no-op.
sbc100 pushed a commit that referenced this issue Aug 6, 2020
This is a followup on #11437 which added this API.  Sadly today the
linker doesn't know the final memory layout so until we fix #11773
this extra `init` function is needed.

The alternative, which is to modify binaryen and emscripten to inject
the stack end value post-link is IMHO way to much complexity to justify
the convenience of not having the call this init function.

Once we fix #11773 the init function can become uneeded/no-op.
sbc100 pushed a commit that referenced this issue Aug 6, 2020
This is a followup on #11437 which added this API.  Sadly today the
linker doesn't know the final memory layout so until we fix #11773
this extra `init` function is needed.

The alternative, which is to modify binaryen and emscripten to inject
the stack end value post-link is IMHO way to much complexity to justify
the convenience of not having the call this init function.

Once we fix #11773 the init function can become uneeded/no-op.
sbc100 pushed a commit that referenced this issue Aug 6, 2020
This is a followup on #11437 which added this API.  Sadly today the
linker doesn't know the final memory layout so until we fix #11773
this extra `init` function is needed.

The alternative, which is to modify binaryen and emscripten to inject
the stack end value post-link is IMHO way to much complexity to justify
the convenience of not having the call this init function.

Once we fix #11773 the init function can become uneeded/no-op.
sbc100 added a commit that referenced this issue Aug 7, 2020
This is a followup on #11437 which added this API.  Sadly today the
linker doesn't know the final memory layout so until we fix #11773
this extra `init` function is needed.

The alternative, which is to modify binaryen and emscripten to inject
the stack end value post-link is IMHO way to much complexity to justify
the convenience of not having the call this init function.

Once we fix #11773 the init function can become unneeded/no-op.
@kripken
Copy link
Member

kripken commented Aug 7, 2020

Was this intentionally closed? I think that PR might have closed the wrong issue @sbc100

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 7, 2020

I think that PR referenced this issue, but I didn't mean to close it.

@sbc100 sbc100 reopened this Aug 7, 2020
@stale
Copy link

stale bot commented Aug 10, 2021

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Aug 10, 2021
@stale stale bot closed this as completed Sep 22, 2021
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