Perform JS static allocations at compile time #7850

kripken · 2019-01-10T23:52:55Z

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 simply 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.

…ywhere (fixes emterpreter, possibly others) [ci skip]

This reverts commit fdda39a.

This reverts commit de1ad42.

…can happen

dschuff · 2019-01-11T18:59:06Z

emscripten.py

@@ -527,6 +527,15 @@ def is_int(x):
    return False


+def align_memory(addr):
+  return (addr + 15) & -16


I find hex literals more readable for bitwise operations but YMMV

dschuff · 2019-01-11T19:01:48Z

emscripten.py

+def apply_forwarded_data(forwarded_data):
+  forwarded_json = json.loads(forwarded_data)
+  # Be aware of JS static allocations
+  shared.Settings.STATIC_BUMP = forwarded_json['STATIC_BUMP']


obviously STATIC_BUMP is preexisting, but it does seem odd to be setting global settings (which usually just come via command-line flags or a certain narrow set of ways to configure) based on this forwarded data.

Actually does this mean that STATIC_BUMP can now be demoted from a shared setting to something local to emscripten.py?

It is slightly odd, yeah, we use shared.Settings as a global location for compiler state. We do mark them in settings.js with comments saying some are just for internal use, like this one. But perhaps they could be in shared.Internals or such?

Specifically here, this can't be a local var to the file since we do want to use it elsewhere (e.g. when we write the metadata we write this - although I didn't update all those places yet in this PR, to keep it small).

dschuff · 2019-01-11T19:09:30Z

emscripten.py

+  static_bump = shared.Settings.STATIC_BUMP
+  #  * then the stack (up on fastcomp, down on upstream)
+  stack_low = align_memory(global_start + static_bump)
+  stack_high = stack_low + shared.Settings.TOTAL_STACK


stack_high should be aligned too, right, at least for wasm backend? are there restrictions on TOTAL_STACK increments?

Good point, I'll align that too.

I don't think we have any checks on the size of the stack, but we could have the compiler error if it's not a multiple if 16 I suppose.

We should probably at least have an assert, a misaligned stack would be an ABI violation.

Makes sense, added that in ASSERTIONS mode at runtime, for the stack (and heap too).

dschuff · 2019-01-11T19:11:46Z

emscripten.py

-  if shared.Settings.SIDE_MODULE or shared.Settings.WASM:
-    pre = pre.replace('{{{ STATIC_BUMP }}}', str(staticbump))
+
+  pre = apply_memory(pre, metadata)


is the new logic still correct for sidemodules and/or !WASM? I don't see any mentions of those.

Identical for wasm and !wasm.

In a side module we wouldn't have STACK_BASE etc., as the location is determined at runtime anyhow (by the dynamic loader). So maybe it's a little odd we compute those even though they are not written out. I'll add a comment.

dschuff

otherwise LGTM

rianhunter · 2019-01-11T21:43:51Z

This should probably increment EMSCRIPTEN_ABI_MINOR (while EMSCRIPTEN_ABI_MAJOR is still 0).

kripken · 2019-01-11T21:46:24Z

@rianhunter I'm not sure this changes the ABI, yet? If we stop passing DYNAMICTOP_PTR from JS to wasm then that would be a change, but this PR doesn't go that far yet. Or is there something else here?

rianhunter · 2019-01-11T21:48:38Z

@kripken I looked briefly but I thought this might change the exports available to the hosted wasm. If all the same memory-layout exports are available then ignore my comment. Otherwise I understand this only changes how the JS runtime allocates static memory.

kripken · 2019-01-11T21:52:07Z

Ok, yeah, then this should be fine as-is. In fact I verified earlier that the wasm emitted is identical to before this PR - the change is only on the JS side.

Thanks for checking - good to have more eyes on this stuff.

juj · 2019-01-14T12:57:30Z

src/library_pthread.js

@@ -529,7 +528,7 @@ var LibraryPThread = {
  },

  _num_logical_cores__deps: ['emscripten_force_num_logical_cores'],
-  _num_logical_cores: '; if (ENVIRONMENT_IS_PTHREAD) __num_logical_cores = PthreadWorkerInit.__num_logical_cores; else { PthreadWorkerInit.__num_logical_cores = __num_logical_cores = allocate(1, "i32*", ALLOC_STATIC); HEAPU32[__num_logical_cores>>2] = navigator["hardwareConcurrency"] || ' + {{{ PTHREAD_HINT_NUM_CORES }}} + '; }',
+  _num_logical_cores: '; if (ENVIRONMENT_IS_PTHREAD) __num_logical_cores = PthreadWorkerInit.__num_logical_cores; else { PthreadWorkerInit.__num_logical_cores = __num_logical_cores = {{{ makeStaticAlloc(1) }}}; HEAPU32[__num_logical_cores>>2] = navigator["hardwareConcurrency"] || ' + {{{ PTHREAD_HINT_NUM_CORES }}} + '; }',


This should be makeStaticAlloc(4)?

Thanks, yeah, fixing these. (Technically the allocations are stack aligned, so 1 means at least 4 anyhow, but better not to rely on that.)

juj · 2019-01-14T12:57:46Z

src/library_pthread.js

@@ -1031,7 +1030,7 @@ var LibraryPThread = {
  },

  // Stores the memory address that the main thread is waiting on, if any.
-  _main_thread_futex_wait_address: '; if (ENVIRONMENT_IS_PTHREAD) __main_thread_futex_wait_address = PthreadWorkerInit.__main_thread_futex_wait_address; else PthreadWorkerInit.__main_thread_futex_wait_address = __main_thread_futex_wait_address = allocate(1, "i32*", ALLOC_STATIC)',
+  _main_thread_futex_wait_address: '; if (ENVIRONMENT_IS_PTHREAD) __main_thread_futex_wait_address = PthreadWorkerInit.__main_thread_futex_wait_address; else PthreadWorkerInit.__main_thread_futex_wait_address = __main_thread_futex_wait_address = {{{ makeStaticAlloc(1) }}}',


makeStaticAlloc(4) as well?

juj · 2019-01-14T13:00:32Z

src/parseTools.js

+}
+
+function makeStaticString(string) {
+  var len = string.length + 1;


This is not correct size computation for an UTF-8 string - either need to use lengthBytesUTF8(string) or if you want to conservatively allocate, use string.length*4+1.

Thanks! Fixing with lengthBytesUTF8 at compile time, to not waste any space here.

juj · 2019-01-14T13:02:17Z

tests/test_core.py

@@ -3790,7 +3790,7 @@ def test_dylink_jslib(self):
  def test_dylink_global_var_jslib(self):
    create_test_file('lib.js', r'''
      mergeInto(LibraryManager.library, {
-        jslib_x: 'allocate(1, "i32*", ALLOC_STATIC)',
+        jslib_x: '{{{ makeStaticAlloc(1) }}}',


makeStaticAlloc(4)?

juj · 2019-01-14T13:04:07Z

src/library_pthread.js

@@ -54,7 +54,7 @@ var LibraryPThread = {

 #if PTHREADS_PROFILING
    createProfilerBlock: function(pthreadPtr) {
-      var profilerBlock = (pthreadPtr == PThread.mainThreadBlock) ? allocate({{{ C_STRUCTS.thread_profiler_block.__size__ }}}, "i32*", ALLOC_STATIC) : _malloc({{{ C_STRUCTS.thread_profiler_block.__size__ }}});
+      var profilerBlock = (pthreadPtr == PThread.mainThreadBlock) ? {{{ makeStaticAlloc(C_STRUCTS.thread_profiler_block.__size__) }}} : _malloc({{{ C_STRUCTS.thread_profiler_block.__size__ }}});


(it looks like the above three code blocks have historically overallocated 4x the number of bytes needed, but this change fixes that)

src/settings.js

Its been more than a year since this was removed in #7850.

kripken added 28 commits January 9, 2019 13:54

very wip

0bdd47b

fix

e5bbc52

remove staticAlloc/ALLOC_STATIC

afc4c16

fixes [ci skip]

ae0bbb5

more [ci skip]

2e75cd8

do dynamic top emitting earlier in the js, so dynamicAlloc works ever…

4a62b1f

…ywhere (fixes emterpreter, possibly others) [ci skip]

new model [ci skip]

35282e2

fix [ci skip]

30f2ff0

test fix

d6b4c00

forward static bump from js to emscripten.py [ci skip]

387034f

fix static strings [ci skip]

b07a654

fixes [ci skip]

1c57b5d

fixes [ci skip]

fdda39a

Revert "fixes [ci skip]"

de1ad42

This reverts commit fdda39a.

Revert "Revert "fixes [ci skip]""

8f7ce9c

This reverts commit de1ad42.

fix [ci skip]

ecedef6

test fix [ci skip]

0338565

remove fixmes

6d62a03

proper allocation

a470a71

work [ci skip]

9d5c349

fix

a07858f

fix zlib compiler error

214f936

fix logger

afbac79

fix pthreads

332849f

remove irrelevant test - without full stack overflow tests, anything …

6d8a2c1

…can happen

merge

23ec68e

cleanup

dccd4f1

cleanup

c6e3ea3

kripken requested review from juj and dschuff January 10, 2019 23:52

dschuff reviewed Jan 11, 2019

View reviewed changes

kripken added 2 commits January 11, 2019 12:51

feedback

c8aed5d

Merge remote-tracking branch 'origin/incoming' into simplify-memory

080820d

dschuff approved these changes Jan 11, 2019

View reviewed changes

assert that the stack and heap must be aligned

4b48ca9

juj reviewed Jan 14, 2019

View reviewed changes

review feedback

9e44fd6

kripken merged commit d9529ae into incoming Jan 14, 2019

kripken deleted the simplify-memory branch January 14, 2019 18:19

sbc100 reviewed Mar 7, 2019

View reviewed changes

src/settings.js Show resolved Hide resolved

sbc100 added a commit that referenced this pull request May 21, 2020

Remove reference to staticAllotc

2fa66b5

Its been more than a year since this was removed in #7850.

sbc100 mentioned this pull request May 21, 2020

Remove reference to staticAlloc #11210

Merged

sbc100 added a commit that referenced this pull request May 21, 2020

Remove reference to staticAlloc

dc7a584

Its been more than a year since this was removed in #7850.

sbc100 added a commit that referenced this pull request May 22, 2020

Remove reference to staticAlloc (#11210)

72d3932

Its been more than a year since this was removed in #7850.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform JS static allocations at compile time #7850

Perform JS static allocations at compile time #7850

kripken commented Jan 10, 2019

dschuff Jan 11, 2019

dschuff Jan 11, 2019

dschuff Jan 11, 2019

kripken Jan 11, 2019

dschuff Jan 11, 2019

kripken Jan 11, 2019

dschuff Jan 11, 2019

kripken Jan 11, 2019

dschuff Jan 11, 2019

kripken Jan 11, 2019

dschuff left a comment

rianhunter commented Jan 11, 2019

kripken commented Jan 11, 2019

rianhunter commented Jan 11, 2019

kripken commented Jan 11, 2019

juj Jan 14, 2019

kripken Jan 14, 2019

juj Jan 14, 2019

juj Jan 14, 2019

kripken Jan 14, 2019

juj Jan 14, 2019

juj Jan 14, 2019

Perform JS static allocations at compile time #7850

Perform JS static allocations at compile time #7850

Conversation

kripken commented Jan 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff left a comment

Choose a reason for hiding this comment

rianhunter commented Jan 11, 2019

kripken commented Jan 11, 2019

rianhunter commented Jan 11, 2019

kripken commented Jan 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment