-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Make sbrk threadsafe #9438
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
Make sbrk threadsafe #9438
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,30 +27,57 @@ extern size_t emscripten_get_heap_size(void); | |
| #endif | ||
|
|
||
| void *sbrk(intptr_t increment) { | ||
| intptr_t* sbrk_ptr = emscripten_get_sbrk_ptr(); | ||
| intptr_t old_brk = *sbrk_ptr; | ||
| // TODO: overflow checks | ||
| intptr_t new_brk = old_brk + increment; | ||
| #if __wasm__ | ||
| uintptr_t old_size = __builtin_wasm_memory_size(0) * WASM_PAGE_SIZE; | ||
| #if __EMSCRIPTEN_PTHREADS__ | ||
| // 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. | ||
| while (1) { | ||
| #endif // __EMSCRIPTEN_PTHREADS__ | ||
|
|
||
| intptr_t* sbrk_ptr = emscripten_get_sbrk_ptr(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this is an existing issue (and it's definitely a nit), but it might be clearer to name this
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, yeah, maybe
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is essentially a private API, then it may not be worth it for that. And if it's a longstanding public API, then maybe we don't want to break it :)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a private api basically, that communicates between emscripten and binaryen, but nothing else should be aware of it. |
||
| #if __EMSCRIPTEN_PTHREADS__ | ||
| intptr_t old_brk = __c11_atomic_load((_Atomic(intptr_t)*)sbrk_ptr, __ATOMIC_SEQ_CST); | ||
| #else | ||
| intptr_t old_brk = *sbrk_ptr; | ||
| #endif | ||
| // TODO: overflow checks | ||
| intptr_t new_brk = old_brk + increment; | ||
| #ifdef __wasm__ | ||
| uintptr_t old_size = __builtin_wasm_memory_size(0) * WASM_PAGE_SIZE; | ||
| #else | ||
| uintptr_t old_size = emscripten_get_heap_size(); | ||
| uintptr_t old_size = emscripten_get_heap_size(); | ||
| #endif | ||
| // TODO In a multithreaded build dlmalloc uses locks around each malloc/free, | ||
| // which means we don't need to use atomics here. In theory however | ||
| // someone could use sbrk outside of dlmalloc in a racy manner. | ||
| if (new_brk > old_size) { | ||
| // Try to grow memory. | ||
| intptr_t diff = new_brk - old_size; | ||
| if (!emscripten_resize_heap(new_brk)) { | ||
| if (new_brk > old_size) { | ||
| // Try to grow memory. | ||
| intptr_t diff = new_brk - old_size; | ||
| if (!emscripten_resize_heap(new_brk)) { | ||
| #ifndef EMSCRIPTEN_NO_ERRNO | ||
| errno = ENOMEM; | ||
| errno = ENOMEM; | ||
| #endif | ||
| return (void*)-1; | ||
| return (void*)-1; | ||
| } | ||
| } | ||
| #if __EMSCRIPTEN_PTHREADS__ | ||
| // 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. | ||
| intptr_t expected = old_brk; | ||
| __c11_atomic_compare_exchange_strong( | ||
| (_Atomic(intptr_t)*)sbrk_ptr, | ||
| &expected, new_brk, | ||
| __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); | ||
| if (expected != old_brk) { | ||
| continue; | ||
| } | ||
| #else // __EMSCRIPTEN_PTHREADS__ | ||
| *sbrk_ptr = new_brk; | ||
| #endif // __EMSCRIPTEN_PTHREADS__ | ||
| return (void*)old_brk; | ||
|
|
||
| #if __EMSCRIPTEN_PTHREADS__ | ||
| } | ||
| *sbrk_ptr = new_brk; | ||
| return (void*)old_brk; | ||
| #endif // __EMSCRIPTEN_PTHREADS__ | ||
| } | ||
|
|
||
| int brk(intptr_t ptr) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing code in emscripten seems inconsistent about whether it should be this or
#ifdef __EMSCRIPTEN_PTHREADS__🤷♀️There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I copied this from dlmalloc, but I'm not surprised other code does differently... I don't feel strongly either way.