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.

Sign up for GitHub

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

Jump to bottom

enable thread safety for sbrk and emmalloc, when using WASM_WORKERS #18174

Merged
merged 4 commits into from
Nov 14, 2022
Merged

enable thread safety for sbrk and emmalloc, when using WASM_WORKERS #18174

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
298c5b5
enable thread safety for sbrk and emmalloc, when using WASM_WORKERS
Nov 10, 2022
529b8e3
test/code_size/hello_wasm_worker_wasm.json: rebaseline
Nov 10, 2022
1fa4e7b
enable thread safety for sbrk and emmalloc, when using WASM_WORKERS -…
Nov 13, 2022
fb4fbde
enable thread safety for sbrk and emmalloc, when using WASM_WORKERS -…
Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion system/lib/emmalloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ typedef struct RootRegion
uint8_t* endPtr;
} RootRegion;

#if defined(__EMSCRIPTEN_PTHREADS__)
#ifdef __EMSCRIPTEN_SHARED_MEMORY__
// In multithreaded builds, use a simple global spinlock strategy to acquire/release access to the memory allocator.
static volatile uint8_t multithreadingLock = 0;
#define MALLOC_ACQUIRE() while(__sync_lock_test_and_set(&multithreadingLock, 1)) { while(multithreadingLock) { /*nop*/ } }
Expand Down
24 changes: 14 additions & 10 deletions system/lib/sbrk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@
*
*/

#ifdef __EMSCRIPTEN_SHARED_MEMORY__
#define RETRY_SBRK 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combining these into a single block makes sense to me.

Also, perhaps instead of defined(__EMSCRIPTEN_PTHREADS__) || defined(__EMSCRIPTEN_WASM_WORKERS__) you can just use ifdef __EMSCRIPTEN_SHARED_MEMORY__ ? Or equivalently ifdef _REENDTRANT

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm well, I suppose this could now just read #ifdef __EMSCRIPTEN_SHARED_MEMORY__ everywhere without introducing a RETRY_SBRK define at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I had thought to keep it for the brevity (since EMSCRIPTEN_SHARED_MEMORY is kind of long), and maybe for future use. Anyway, changed to #ifdef __EMSCRIPTEN_SHARED_MEMORY__ as you suggested.

#endif

#ifndef EMSCRIPTEN_NO_ERRNO
#include <errno.h>
#endif
#include <limits.h>
#include <stddef.h>
#include <stdint.h>
#if __EMSCRIPTEN_PTHREADS__ // for error handling, see below
#if RETRY_SBRK // for error handling, see below
#include <stdio.h>
#include <stdlib.h>
#endif
Expand Down Expand Up @@ -54,16 +58,16 @@ void *sbrk(intptr_t increment_) {
uintptr_t old_size;
uintptr_t increment = (uintptr_t)increment_;
increment = (increment + (SBRK_ALIGNMENT-1)) & ~(SBRK_ALIGNMENT-1);
#if __EMSCRIPTEN_PTHREADS__
#if RETRY_SBRK
// Our default dlmalloc uses locks around each malloc/free, so no additional
// work is necessary to keep things threadsafe, but we also make sure sbrk
// itself is threadsafe so alternative allocators work. We do that by looping
// and retrying if we hit interference with another thread.
uintptr_t expected;
while (1) {
#endif // __EMSCRIPTEN_PTHREADS__
#endif // RETRY_SBRK
uintptr_t* sbrk_ptr = emscripten_get_sbrk_ptr();
#if __EMSCRIPTEN_PTHREADS__
#if RETRY_SBRK
uintptr_t old_brk = __c11_atomic_load((_Atomic(uintptr_t)*)sbrk_ptr, __ATOMIC_SEQ_CST);
#else
uintptr_t old_brk = *sbrk_ptr;
Expand All @@ -81,7 +85,7 @@ void *sbrk(intptr_t increment_) {
goto Error;
}
}
#if __EMSCRIPTEN_PTHREADS__
#if RETRY_SBRK
// Attempt to update the dynamic top to new value. Another thread may have
// beat this one to the update, in which case we will need to start over
// by iterating the loop body again.
Expand All @@ -93,26 +97,26 @@ void *sbrk(intptr_t increment_) {
if (expected != old_brk) {
continue;
}
#else // __EMSCRIPTEN_PTHREADS__
#else // RETRY_SBRK
*sbrk_ptr = new_brk;
#endif // __EMSCRIPTEN_PTHREADS__
#endif // RETRY_SBRK

#ifdef __EMSCRIPTEN_TRACING__
emscripten_memprof_sbrk_grow(old_brk, new_brk);
#endif
return (void*)old_brk;

#if __EMSCRIPTEN_PTHREADS__
#if RETRY_SBRK
}
#endif // __EMSCRIPTEN_PTHREADS__
#endif // RETRY_SBRK

Error:
SET_ERRNO();
return (void*)-1;
}

int brk(void* ptr) {
#if __EMSCRIPTEN_PTHREADS__
#if RETRY_SBRK
// FIXME
printf("brk() is not theadsafe yet, https://github.com/emscripten-core/emscripten/issues/10006");
abort();
Expand Down
4 changes: 2 additions & 2 deletions test/code_size/hello_wasm_worker_wasm.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
"a.html.gz": 433,
"a.js": 733,
"a.js.gz": 463,
"a.wasm": 1805,
"a.wasm.gz": 1002,
"a.wasm": 1861,
"a.wasm.gz": 1031,
"total": 3275,
"total_gz": 1898
}