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

Simplify memory setup / remove special JS allocation modes? #7795

Closed
kripken opened this issue Jan 2, 2019 · 13 comments
Closed

Simplify memory setup / remove special JS allocation modes? #7795

kripken opened this issue Jan 2, 2019 · 13 comments
Labels

Comments

@kripken
Copy link
Member

kripken commented Jan 2, 2019

I'd like to simplify our memory setup code by removing the ability to do "special" allocations in JS. That is, right now you can do these allocations from JS:

  • Stack allocation (calls stackAlloc in compiled code)
  • Heap/normal allocation (calls malloc in compiled code)
  • Static allocation (done in JS during startup, before static memory is "sealed")
  • Dynamic allocation (done in JS during startup, after static memory is "sealed", from the area sbrk uses)

I propose keeping the first two (which we can't remove, they are the absolute minimum) and removing the latter two.

The benefits of removing them:

  • Remove a bunch of JS setup/startup code.
  • Simplify the overall model of how memory is set up, less to think about.
  • Allow moving all the memory setup and management logic into compiled code.
  • Allow further optimizations like the ctor evaller doing mallocs at compile time.
  • Simplify our JS/wasm ABI, that is, make our wasm more standalone and easier to run without special JS.

The downsides:

  • No more JS allocation of memory in a synchronous way during startup - since compiled code will manage memory, and we must call into it for malloc/stackAlloc, we must wait for compiled code to be ready first.
  • Existing JS features that depend on the existing APIs need some work, and in particular some may need to become async. This may actually increase code size somewhat for users of those features.
  • Existing external (not in-tree) users of those internal allocation APIs will break. We've never really recommended these APIs, and they are marked as internal, e.g. http://kripken.github.io/emscripten-site/docs/api_reference/advanced-apis.html?highlight=allocate#allocate , so hopefully there are not many such users. Opening this issue to discuss that.
@dschuff
Copy link
Member

dschuff commented Jan 9, 2019

I regret that I have but one +1 to give for this proposal....

@kripken
Copy link
Member Author

kripken commented Jan 9, 2019

@juj, any thoughts/concerns here before I start to look into this?

@juj
Copy link
Collaborator

juj commented Jan 9, 2019

How do you see the extent of this to go:

  • Does this mean that the STATICTOP variable would be removed?
  • How about the staticSealed model? Would that go away?
  • Will the bottom address of the dynamic memory area (DYNAMIC_BASE) then be dictated by compiled code up front?

The sbrk() model of growing dynamic memory area is still intended to stay, I suppose? (I do like that model, wish we'd have that paired with page unloading in the future, and/or sbrk() shrinking aware malloc that we could experiment with freeing committed physical memory at runtime)

The allocate() function is great to be removed, we do want to have a code flow that statically/at compile time knows what kind of allocations are being performed.

Although I think there may still be a need to have a staticAlloc() function that is able to allocate bytes from .js code by bumping STATICTOP before startup? If that can be removed with a better model that does not grow code size and is not constraining, especially with multithreading in mind, then I don't mind if staticAlloc() is removed. (If a replacement for staticAlloc() would end up to be some kind of asynchronous(?) post-globals-init model that takes up more code size than current form, then that may be troublesome)

@kripken
Copy link
Member Author

kripken commented Jan 9, 2019

Does this mean that the STATICTOP variable would be removed?

Yes, JS would no longer manage any memory allocation. It would just call into wasm/asm.js for stackAlloc or malloc. (should be a nice shrinking of our JS glue)

How about the staticSealed model? Would that go away?

Yes.

Will the bottom address of the dynamic memory area (DYNAMIC_BASE) then be dictated by compiled code up front?

Yes.

The sbrk() model of growing dynamic memory area is still intended to stay, I suppose?

Yes. Still sbrk() as normal, but done all in compiled code (it already mostly is, just the initial location came from JS).

The allocate() function is great to be removed, we do want to have a code flow that statically/at compile time knows what kind of allocations are being performed.

Note that allocate may be orthogonal to this issue: it may stay around, but just support stack and malloc allocation, removing 2/4 of the modes. So it would be simplified. But yes, I agree we should look at removing it separately.

But yes, as you said in the second part of your comment, statically handling JS static allocations may be useful here, and may end up replacing some allocate()s (the static and perhaps dynamic ones).

Although I think there may still be a need to have a staticAlloc() function that is able to allocate bytes from .js code by bumping STATICTOP before startup? If that can be removed with a better model that does not grow code size and is not constraining, especially with multithreading in mind, then I don't mind if staticAlloc() is removed. (If a replacement for staticAlloc() would end up to be some kind of asynchronous(?) post-globals-init model that takes up more code size than current form, then that may be troublesome)

Yes, this may end up the hard part here - it may need to be asynchronous if it is to be in JS. Other options are to statically compute JS static allocations, as just mentioned. Another option is for me to move some things into compiled code. I hope to avoid any async messiness with both of those, but I can't be sure til I try.

@rianhunter
Copy link
Contributor

rianhunter commented Jan 9, 2019 via email

@juj
Copy link
Collaborator

juj commented Jan 10, 2019

I am in favor of removing STATICTOP, staticSealed and staticAlloc, and allocate. It feels like there is a great flexibility that is provided if JS code can decide where the dynamic memory area starts, when it is compiling a WebAssembly module, i.e. JS code would define DYNAMIC_BASE/DYNAMICTOP_PTR, and pass it off as an import to the WebAssembly module? That way JS code will have the power of bumping up DYNAMIC_BASE/DYNAMICTOP_PTR synchronously before it starts to compile, in case there will be a need to add blocks of memory to the heap at startup.

If instead DYNAMIC_BASE/DYNAMICTOP_PTR is burned in to the WebAssembly module at compilation time, it may make JS side too inflexible to manage startup process on the heap, and it may need to awkwardly split up its initialization in multiple parts. Two examples that come to mind are multithreaded startup logic, where main thread allocates pointers to the heap to share info to pthreads, and MEMFS&ASMFS files preloading on the heap before application start. Does it gain anything to have Wasm module side establish the starting value of the dynamic memory area at compilation time?

@kripken
Copy link
Member Author

kripken commented Jan 10, 2019

Does it gain anything to have Wasm module side establish the starting value of the dynamic memory area at compilation time?

Yes, it allows the optimizer to pre-execute code at compile time. Say if the program starts with a few mallocs, those can be done at compile time, improving startup and download times.

Two examples that come to mind are multithreaded startup logic, where main thread allocates pointers to the heap to share info to pthreads,

How many does it allocate? If it's just a few, then statically allocating them at compile time seems ok?

MEMFS&ASMFS files preloading on the heap before application start.

That is async anyhow, and so it seems like it could use malloc - so it just waits for the wasm to be ready to run?

kripken added a commit that referenced this issue Jan 14, 2019
First part of #7795: this removes the ability to do static allocations in JS when the app starts. That means no more staticAlloc, ALLOC_STATIC, and the staticSealed logic to handle it. Instead, static allocations are done at compile time, using makeStaticAlloc. The JS compiler informs emscripten.py of those static allocations, and then they are added after static allocations from LLVM, basically.

Benefits:

* Remove a bunch of complexity (no more staticSealed), etc. See apply_memory in emscripten.py for how simple memory layout is after this: we run the C compiler, then the JS compiler, then in that python code we can lay out static memory completely at compile time.
* Saves code size and startup work by replacing x = staticAlloc(size) to x = ptr (hardcoded ptr numbers, computed at compile time). On hello world (with closure/-Os) this saves around 1%, more than I thought actually.

Downsides:

* The current model is very simple: JS library files do {{{ makeStaticAlloc(size) }}} which does the allocation and returns a ptr, at compile time. However, when JS did the call at runtime, if the code was not reached the allocation was not done. In this model, if the JS library is parsed and preprocessed, we allocate - which means the library_pthreads.js ones are not done by default, but the library.js ones are. As a result, we are wasting in some cases a little static space that is actually unused (maybe 100 bytes?). This is probably pretty negligible, but we could optimize it out later, at the cost of more complexity in the JS compiler - not sure if that would be worth it.

Possible followups / stuff not done in this PR:

* No changes to dynamic allocation in JS when the app starts. Only static allocation is changed here.
We could hardcode DYNAMICTOP_PTR's location in the wasm (but not its value, not without changes to dynamic allocation, see previous point), say as a param to wasm-emscripten-finalize etc. This would save passing it at runtime into the wasm.

PR also contains a few minor test fixes, like the stack-overflow test had an incorrect part, but we never noticed til now where the new memory layout happens to make it actually cause a problem.
kripken added a commit that referenced this issue Jan 16, 2019
Part of #7795. Depends on WebAssembly/binaryen#1870 (has a hardcoded binaryen port value for testing, before landing needs to be a new tag there)

This is the emscripten side of that PR:

* For asm2wasm, define STACKTOP/STACK_MAX in the asm.js code with the hardcoded value directly.
* For wasm-emscripten-finalize, send it using --initial-stack-pointer.

This saves 1 or 2 imports in the wasm, and JS to send it there. Not a huge savings in code size (16 bytes in hello world), but it is simpler and also simpler in the compiler code too.

* bump abi

* fix test_stack_varargs - in the wasm backend, 2K is not enough for the single printf, it overflows. double the stack and double the stack uses

* update binaryen port
@kripken
Copy link
Member Author

kripken commented Jan 24, 2019

ping @juj - see comments above.

@juj
Copy link
Collaborator

juj commented Jan 29, 2019

sgtm

@stale
Copy link

stale bot commented Jan 29, 2020

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 7 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Jan 29, 2020
@kripken
Copy link
Member Author

kripken commented Jan 30, 2020

I still think this is worth doing.

@stale
Copy link

stale bot commented Jan 30, 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 Jan 30, 2021
@stale stale bot closed this as completed Mar 20, 2021
@kripken
Copy link
Member Author

kripken commented Mar 20, 2021

I believe this has been done.

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

No branches or pull requests

4 participants