Simplify GROWABLE_HEAP transform

site/source/docs/porting/pthreads.rst

@@ -144,7 +144,7 @@ The Emscripten implementation for the pthreads API should follow the POSIX stand
 
 - Note that the function emscripten_num_logical_cores() will always return the value of navigator.hardwareConcurrency, i.e. the number of logical cores on the system, even when shared memory is not supported. This means that it is possible for emscripten_num_logical_cores() to return a value greater than 1, while at the same time emscripten_has_threading_support() can return false. The return value of emscripten_has_threading_support() denotes whether the browser has shared memory support available.
 
-- Pthreads + memory growth (``ALLOW_MEMORY_GROWTH``) is especially tricky, see `Wasm design issue #1271 <https://github.com/WebAssembly/design/issues/1271>`_. This currently causes JS accessing the Wasm memory to be slow - but this will likely only be noticeable if the JS does large amounts of memory reads and writes (Wasm runs at full speed, so moving work over can fix this). This also requires that your JS be aware that the HEAP* views may need to be updated - JS code embedded with ``--js-library`` etc will automatically be transformed to use the ``GROWABLE_HEAP_*`` helper functions where ``HEAP*`` are used, but external code that uses ``Module.HEAP*`` directly may encounter problems with views being smaller than memory.
+- Pthreads + memory growth (``ALLOW_MEMORY_GROWTH``) is especially tricky, see `Wasm design issue #1271 <https://github.com/WebAssembly/design/issues/1271>`_. This currently causes JS accessing the Wasm memory to be slow - but this will likely only be noticeable if the JS does large amounts of memory reads and writes (Wasm runs at full speed, so moving work over can fix this). This also requires that your JS be aware that the HEAP* views may need to be updated - JS code embedded with ``--js-library`` etc will automatically be transformed to auto-update memory views before each access, but external code that uses ``Module.HEAP*`` directly may encounter problems with views being smaller than memory.
 
 .. _Allocator_performance:
 

src/growableHeap.js

@@ -6,63 +6,9 @@
 
 // Support for growable heap + pthreads, where the buffer may change, so JS views
 // must be updated.
-function GROWABLE_HEAP_I8() {
+function growMemViews() {
+  // `updateMemoryViews` updates all the views simultaneously, so it's enough to check any of them.
   if (wasmMemory.buffer != HEAP8.buffer) {
     updateMemoryViews();
   }
-  return HEAP8;
-}
-function GROWABLE_HEAP_U8() {
-  if (wasmMemory.buffer != HEAP8.buffer) {
-    updateMemoryViews();
-  }
-  return HEAPU8;
-}
-function GROWABLE_HEAP_I16() {
-  if (wasmMemory.buffer != HEAP8.buffer) {
-    updateMemoryViews();
-  }
-  return HEAP16;
-}
-function GROWABLE_HEAP_U16() {
-  if (wasmMemory.buffer != HEAP8.buffer) {
-    updateMemoryViews();
-  }
-  return HEAPU16;
-}
-function GROWABLE_HEAP_I32() {
-  if (wasmMemory.buffer != HEAP8.buffer) {
-    updateMemoryViews();
-  }
-  return HEAP32;
-}
-function GROWABLE_HEAP_U32() {
-  if (wasmMemory.buffer != HEAP8.buffer) {
-    updateMemoryViews();
-  }
-  return HEAPU32;
-}
-function GROWABLE_HEAP_I64() {
-  if (wasmMemory.buffer != HEAP8.buffer) {
-    updateMemoryViews();
-  }
-  return HEAP64;
-}
-function GROWABLE_HEAP_U64() {
-  if (wasmMemory.buffer != HEAP8.buffer) {
-    updateMemoryViews();
-  }
-  return HEAPU64;
-}
-function GROWABLE_HEAP_F32() {
-  if (wasmMemory.buffer != HEAP8.buffer) {
-    updateMemoryViews();
-  }
-  return HEAPF32;
-}
-function GROWABLE_HEAP_F64() {
-  if (wasmMemory.buffer != HEAP8.buffer) {
-    updateMemoryViews();
-  }
-  return HEAPF64;
 }

src/lib/libpthread.js

@@ -979,11 +979,6 @@ var LibraryPThread = {
   $establishStackSpace__internal: true,
   $establishStackSpace__deps: ['$stackRestore', 'emscripten_stack_set_limits'],
   $establishStackSpace: (pthread_ptr) => {
-#if ALLOW_MEMORY_GROWTH
-    // If memory growth is enabled, the memory views may have gotten out of date,
-    // so resync them before accessing the pthread ptr below.
-    updateMemoryViews();
-#endif
     var stackHigh = {{{ makeGetValue('pthread_ptr', C_STRUCTS.pthread.stack, '*') }}};
     var stackSize = {{{ makeGetValue('pthread_ptr', C_STRUCTS.pthread.stack_size, '*') }}};
     var stackLow = stackHigh - stackSize;

src/modules.mjs

@@ -455,20 +455,6 @@ function exportRuntimeSymbols() {
     runtimeElements.push('HEAP_DATA_VIEW');
   }
 
-  if (PTHREADS && ALLOW_MEMORY_GROWTH) {
-    runtimeElements.push(
-      'GROWABLE_HEAP_I8',
-      'GROWABLE_HEAP_U8',
-      'GROWABLE_HEAP_I16',
-      'GROWABLE_HEAP_U16',
-      'GROWABLE_HEAP_I32',
-      'GROWABLE_HEAP_U32',
-      'GROWABLE_HEAP_I64',
-      'GROWABLE_HEAP_U64',
-      'GROWABLE_HEAP_F32',
-      'GROWABLE_HEAP_F64',
-    );
-  }
   if (USE_OFFSET_CONVERTER) {
     runtimeElements.push('WasmOffsetConverter');
   }

test/js_optimizer/growableHeap-output.js

@@ -1,13 +1,13 @@
 function f() {
   var $2 = 0;
-  GROWABLE_HEAP_I32()[$0 >> 2] = $2 + 1;
-  $9 = GROWABLE_HEAP_U8()[$2 >> 0] | 0;
-  +GROWABLE_HEAP_F64()[x >> 3];
-  GROWABLE_HEAP_I64()[x >> 3] = GROWABLE_HEAP_I64()[y >> 3];
+  (growMemViews(), HEAP32)[$0 >> 2] = $2 + 1;
+  $9 = (growMemViews(), HEAPU8)[$2 >> 0] | 0;
+  +(growMemViews(), HEAPF64)[x >> 3];
+  (growMemViews(), HEAP64)[x >> 3] = (growMemViews(), HEAP64)[y >> 3];
 }
 
 function libraryFunc(ptr, val) {
-  if (ptr < GROWABLE_HEAP_I8().length) {
-    Atomics.wait(GROWABLE_HEAP_I32(), ptr, val);
+  if (ptr < (growMemViews(), HEAP8).length) {
+    Atomics.wait((growMemViews(), HEAP32), ptr, val);
   }
 }

test/other/codesize/test_codesize_minimal_pthreads_memgrowth.exports

@@ -0,0 +1,17 @@
+A (_emscripten_thread_exit)
+B (_emscripten_check_mailbox)
+C (emscripten_stack_set_limits)
+D (_emscripten_stack_restore)
+E (_emscripten_stack_alloc)
+F (emscripten_stack_get_current)
+p (__wasm_call_ctors)
+q (add)
+r (main)
+s (__indirect_function_table)
+t (_emscripten_tls_init)
+u (pthread_self)
+v (_emscripten_proxy_main)
+w (_emscripten_thread_init)
+x (_emscripten_thread_crashed)
+y (_emscripten_run_on_main_thread_js)
+z (_emscripten_thread_free_data)

test/other/codesize/test_codesize_minimal_pthreads_memgrowth.funcs

@@ -0,0 +1,91 @@
+$__emscripten_stdout_seek
+$__errno_location
+$__memcpy
+$__memset
+$__pthread_getspecific
+$__pthread_mutex_lock
+$__pthread_mutex_trylock
+$__pthread_mutex_unlock
+$__pthread_rwlock_timedrdlock
+$__pthread_rwlock_tryrdlock
+$__pthread_rwlock_trywrlock
+$__pthread_rwlock_unlock
+$__pthread_self_internal
+$__pthread_setcancelstate
+$__set_thread_state
+$__stdio_write
+$__timedwait
+$__timedwait_cp
+$__tl_lock
+$__tl_unlock
+$__wait
+$__wake
+$__wake
+$__wasi_syscall_ret
+$__wasm_call_ctors
+$__wasm_init_memory
+$__wasm_init_tls
+$_emscripten_check_mailbox
+$_emscripten_proxy_main
+$_emscripten_run_on_main_thread_js
+$_emscripten_stack_alloc
+$_emscripten_stack_restore
+$_emscripten_thread_crashed
+$_emscripten_thread_exit
+$_emscripten_thread_free_data
+$_emscripten_thread_init
+$_emscripten_thread_mailbox_init
+$_emscripten_tls_init
+$_emscripten_yield
+$_main_thread
+$a_cas
+$a_cas
+$a_cas_p
+$a_dec
+$a_fetch_add
+$a_fetch_add
+$a_inc
+$a_store
+$a_swap
+$add
+$call_callback_then_free_ctx
+$call_cancel_then_free_ctx
+$call_then_finish_task
+$call_with_ctx
+$cancel_active_ctxs
+$cancel_ctx
+$cancel_notification
+$dispose_chunk
+$do_proxy
+$em_proxying_ctx_deinit
+$em_task_queue_cancel
+$em_task_queue_create
+$em_task_queue_dequeue
+$em_task_queue_enqueue
+$em_task_queue_execute
+$em_task_queue_free
+$em_task_queue_is_empty
+$emscripten_builtin_free
+$emscripten_builtin_malloc
+$emscripten_futex_wait
+$emscripten_futex_wake
+$emscripten_stack_get_current
+$emscripten_stack_set_limits
+$free_ctx
+$get_tasks_for_thread
+$init_active_ctxs
+$init_file_lock
+$init_mparams
+$lock
+$main
+$nodtor
+$pthread_attr_destroy
+$pthread_cond_signal
+$pthread_mutex_destroy
+$pthread_setspecific
+$receive_notification
+$remove_active_ctx
+$run_js_func
+$sbrk
+$undo
+$unlock

test/other/codesize/test_codesize_minimal_pthreads_memgrowth.gzsize

@@ -0,0 +1 @@
+3991

test/other/codesize/test_codesize_minimal_pthreads_memgrowth.imports

@@ -0,0 +1,15 @@
+a (memory)
+b (emscripten_get_now)
+c (exit)
+d (_emscripten_thread_set_strongref)
+e (fd_write)
+f (emscripten_runtime_keepalive_check)
+g (emscripten_resize_heap)
+h (emscripten_exit_with_live_runtime)
+i (emscripten_check_blocking_allowed)
+j (_emscripten_thread_mailbox_await)
+k (_emscripten_thread_cleanup)
+l (_emscripten_receive_on_main_thread_js)
+m (_emscripten_notify_mailbox_postmessage)
+n (_emscripten_init_main_thread_js)
+o (__pthread_create_js)

test/other/codesize/test_codesize_minimal_pthreads_memgrowth.jssize

@@ -0,0 +1 @@
+8263

test/other/codesize/test_codesize_minimal_pthreads_memgrowth.sent

@@ -0,0 +1,15 @@
+a (memory)
+b (emscripten_get_now)
+c (exit)
+d (_emscripten_thread_set_strongref)
+e (fd_write)
+f (emscripten_runtime_keepalive_check)
+g (emscripten_resize_heap)
+h (emscripten_exit_with_live_runtime)
+i (emscripten_check_blocking_allowed)
+j (_emscripten_thread_mailbox_await)
+k (_emscripten_thread_cleanup)
+l (_emscripten_receive_on_main_thread_js)
+m (_emscripten_notify_mailbox_postmessage)
+n (_emscripten_init_main_thread_js)
+o (__pthread_create_js)

test/other/codesize/test_codesize_minimal_pthreads_memgrowth.size

@@ -0,0 +1 @@
+19397

test/pthread/test_pthread_memory_growth.c

@@ -4,42 +4,83 @@
 // found in the LICENSE file.
 
 #include <pthread.h>
-#include <sys/types.h>
-#include <stdio.h>
 #include <stdlib.h>
 #include <assert.h>
-#include <emscripten.h>
+#include <emscripten/em_js.h>
+#include <stdbool.h>
 
-static void *thread_start(void *arg)
+// We want to test that JavaScript access in the main thread automatically
+// updates the memory views if the heap has grown from the pthread meanwhile.
+//
+// Checking this correctly is somewhat tricky because a lot of Emscripten APIs
+// access heap on their own, so the memory views might be updated by them before
+// we explicitly check the state in our own JS routines below, which leads to
+// false positives (test passing even though in isolation the heap access in our
+// own JS is not the one updating the memory views).
+//
+// To test it in isolation, we need to use only EM_JS - which is as close to a
+// pure JS call as we can get - and not EM_ASM, pthread_join, proxying etc., not
+// even `puts`/`printf` for logging - as all of those access the heap from JS on
+// their own and might update the memory views too early. Not using standard
+// proxying mechanisms also means we need to drop down all the way to raw
+// atomics.
+
+EM_JS(void, assert_initial_heap_state, (bool isWorker), {
+  dbg(`Checking initial heap state on the ${isWorker ? 'worker' : 'main'} thread`);
+  assert(HEAP8.length === 32 * 1024 * 1024, "start at 32MB");
+});
+
+EM_JS(void, js_assert_final_heap_state, (bool isWorker, const char* buffer, int finalHeapSize), {
+  dbg(`Checking final heap state on the ${isWorker ? 'worker' : 'main'} thread`);
+  assert(HEAP8.length > finalHeapSize, "end with >64MB");
+  assert(HEAP8[buffer] === 42, "readable from JS");
+});
+
+#define FINAL_HEAP_SIZE (64 * 1024 * 1024)
+
+static char *alloc_beyond_initial_heap()
 {
-  // allocate more memory than we currently have, forcing a growth
-  printf("thread_start\n");
-  char* buffer = (char*)malloc(64 * 1024 * 1024);
+  char *buffer = malloc(FINAL_HEAP_SIZE);
   assert(buffer);
+  // Write value at the end of the buffer to check that any thread can access addresses beyond the initial heap size.
+  buffer += FINAL_HEAP_SIZE - 1;
   *buffer = 42;
-  pthread_exit((void*)buffer);
+  return buffer;
+}
+
+const char *_Atomic buffer = NULL;
+
+static void assert_final_heap_state(bool is_worker)
+{
+  assert(buffer != NULL);
+  assert(*buffer == 42);
+  js_assert_final_heap_state(is_worker, buffer, FINAL_HEAP_SIZE);
+}
+
+static void *thread_start(void *arg)
+{
+  assert_initial_heap_state(true);
+  // allocate more memory than we currently have, forcing a growth
+  buffer = alloc_beyond_initial_heap();
+  assert_final_heap_state(true);
+  return NULL;
 }
 
 int main()
 {
-  printf("prep\n");
+  assert_initial_heap_state(false);
 
   pthread_t thr;
+  int res = pthread_create(&thr, NULL, thread_start, NULL);
+  assert(res == 0);
 
-  printf("start\n");
-  EM_ASM({ assert(HEAP8.length === 32 * 1024 * 1024, "start at 32MB") });
+  while (!buffer);
 
-  printf("create\n");
-  int s = pthread_create(&thr, NULL, thread_start, (void*)NULL);
-  assert(s == 0);
-  void* result = NULL;
-
-  printf("join\n");
-  s = pthread_join(thr, &result);
-  assert(result != 0); // allocation should have succeeded
-  char* buffer = (char*)result;
   assert(*buffer == 42); // should see the value the thread wrote
-  EM_ASM({ assert(HEAP8.length > 64 * 1024 * 1024, "end with >64MB") });
+  assert_final_heap_state(false);
+
+  res = pthread_join(thr, NULL);
+  assert(res == 0);
+
   return 0;
 }
-