-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Working (but slow) support for pthreads + memory growth #8365
Conversation
…ere is a source of nondeterminism in the JS backend
Ah, I see some errors on CI that I missed in the subset of tests I ran locally. I don't see a way to turn this into a draft PR - is there? Anyhow, this is still a wip. |
pthread_t thr; | ||
|
||
printf("start\n"); | ||
EM_ASM({ assert(HEAP8.length === 32 * 1024 * 1024, "start at 32MB") }); |
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.
use EM_JS?
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.
Hmm, it's just test code, and convenient to do it all on a single line? Maybe I don't see the benefit to using EM_JS here?
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.
There's no benefit, other than everything should use EM_JS unless there's a good reason not to?
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.
In general yeah, but it's just not inline, so it's more typing and less immediately obvious to the reader - the only advantages of EM_ASM, basically. I agree EM_JS is the preferred thing for most normal code though.
Ok, I pushed a fix for modularize here. I ran into some MODULARIZE_INSTANCE pre-existing bugs, which I opened #8372 for, and which should land before this. Thanks for testing @Brion! Which In general it's safest to use those special |
Yeah these are in my custom JS library mix-ins, where I either malloc a buffer and write into it or create a typed array view for a C-created buffer to take data out. I'm not doing a lot of individual loads/stores; mostly data's coming as arguments on a callback, some of which are pointers/length pairs for buffers to export, which may be beyond the end of the old buffer views. I think it should work to run It might feel more natural to have view-creation accessors that wrap that check-and-update call, like:
|
Yeah, view creators are a natural fit too, added now. Also added docs about this. |
The view accessors look perfect, thanks! |
src/preamble.js
Outdated
#endif | ||
|
||
#if MODULARIZE | ||
// In modularize mode the wasmMemory and others are received in an onmessage, and that |
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.
should this be "in pthreads mode" rather than "modularize mode"
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.
Fixing comment.
pthread_t thr; | ||
|
||
printf("start\n"); | ||
EM_ASM({ assert(HEAP8.length === 32 * 1024 * 1024, "start at 32MB") }); |
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.
There's no benefit, other than everything should use EM_JS unless there's a good reason not to?
def test_pthread_growth_mainthread(self): | ||
def run(emcc_args=[]): | ||
# Note that this test may not pass on Chrome until 75, due to https://bugs.chromium.org/p/v8/issues/detail?id=9062 | ||
self.btest(path_from_root('tests', 'pthread', 'test_pthread_memory_growth_mainthread.c'), expected='1', args=['-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=2', '-s', 'ALLOW_MEMORY_GROWTH=1', '-s', 'TOTAL_MEMORY=32MB', '-s', 'WASM_MEM_MAX=256MB'] + emcc_args, also_asmjs=False) |
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.
should test in PROXY_TO_PTHREAD mode too?
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.
that won't work yet because of chromium issue 9065, same as test_pthread_growth
. I'll add a PROXY_TO_PTHREAD
mode in that test.
@@ -5887,6 +5887,73 @@ function safeHeap(ast) { | |||
}); | |||
} | |||
|
|||
function growableHeap(ast) { |
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.
should this have some comment like "transform HEAP8 etc heap accesses to calls to GROWABLE_HEAP_LOAD_I8" or something?
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.
Adding a comment.
…main thread can work (and hopefully soon unstable will make the thread version work too)
I tried to use chrome unstable so that at least part of the new tests here could pass, but it fails on unrelated things on circle, so that'll need to be looked at separately. As a result, for now the new tests are disabled. |
…ore#8365) (in wasm - no support in asm.js) Background: WebAssembly/design#1271 * Add GROWABLE_HEAP_* methods that replace each load and store in JS. These safely check if memory grew, and updates the buffer and views if so. The JS optimizer is used to instrument the JS in this way, so it works for user JS as well. * Show a warning about the slowness during compilation. * Make sbrk etc. threadsafe. * Rewrite brk using sbrk, to avoid writing two pieces of threadsafe code.
…ripten-core#8365)" This reverts commit a8164ac.
…ripten-core#8365)" This reverts commit a8164ac.
…ore#8365) (in wasm - no support in asm.js) Background: WebAssembly/design#1271 * Add GROWABLE_HEAP_* methods that replace each load and store in JS. These safely check if memory grew, and updates the buffer and views if so. The JS optimizer is used to instrument the JS in this way, so it works for user JS as well. * Show a warning about the slowness during compilation. * Make sbrk etc. threadsafe. * Rewrite brk using sbrk, to avoid writing two pieces of threadsafe code.
(in wasm - no support in asm.js)
Background: WebAssembly/design#1271
GROWABLE_HEAP_*
methods that replace each load and store in JS. These safely check if memory grew, and updates the buffer and views if so. The JS optimizer is used to instrument the JS in this way, so it works for user JS as well.emscripten_resize_heap
to the main thread, and handle races where a thread asks for growth but another thread grows it to a lower or higher value meanwhile - loop until successful, or an error is hit.Blocked on https://bugs.chromium.org/p/v8/issues/detail?id=9062 , so this is not fully tested yet - it doesn't test an actual memory growth, but it does test that instrumenting the code works.