MEMORY64 + pthreads support in browser#19959
MEMORY64 + pthreads support in browser#19959dakenf wants to merge 3 commits intoemscripten-core:mainfrom
Conversation
| // was the same as calling _emscripten_yield() | ||
| // a few times before calling emscripten_futex_wait(). | ||
| if (__c11_atomic_load((_Atomic uintptr_t*)addr, __ATOMIC_SEQ_CST) != val) { | ||
| if (__c11_atomic_load((_Atomic uint32_t*)addr, __ATOMIC_SEQ_CST) != val) { |
There was a problem hiding this comment.
This feels wrong but it's the only way I could get it working. I've tried changing argument type to uintptr_t but it fails with "i64.atomic.load: this operation does not support unaligned access". Most likely it needs to be fixed somewhere else
There was a problem hiding this comment.
Since val is uint32_t I think this actually makes sense. I don't think we ever want this to compile to an i64 operation.
We might want to change the first argument to emscripten_futex_wait to be uint32_t* too.
test/test_browser.py
Outdated
| def test_emscripten_performance_now(self): | ||
| self.btest(test_file('emscripten_performance_now.c'), '0', args=['-pthread', '-sPROXY_TO_PTHREAD']) | ||
| self.emcc_args += ['-Wno-experimental', '-pthread', '-Wno-pthreads-mem-growth'] | ||
| self.btest(test_file('emscripten_performance_now.c'), '0', args=['-pthread', '-sMEMORY64', '-sPROXY_TO_PTHREAD', '-sINITIAL_MEMORY=67108864']) |
There was a problem hiding this comment.
Should this be with @also_with_wasm64 also too.. so we test both.
(it seems odd to use both emcc_args += and args= in the same test. maybe just stick to one or the other)
There was a problem hiding this comment.
Yeah, I forgot to revert changes I made while testing
src/runtime_init_memory.js
Outdated
| 'shared': true, | ||
| #endif | ||
| #if MEMORY64 | ||
| 'index': 'u64', |
There was a problem hiding this comment.
Can you align this with 'shared' above. Also, move the trailing comma from line 43 up so that its on the maximum line.
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Awesome work.
Can we land some of these changes without the 'index': 'u64', change (so we don't need to wait for the v8 change?)
Unfortunately no. Anything compiled with thread local storage will throw an error "wasm streaming compile failed: RuntimeError: memory access out of bounds". It calls memory.init with i64.const destination which is not handled in V8 |
I'm a little confused how the pthread tests in test_core.py are running under node then. For example |
Well, it looks like they use completely different V8 code (an outdated one that worked fine?): https://github.com/nodejs/node/blob/fb47afc335ef78a8cef7eac52b8ee7f045300696/deps/v8/src/wasm/baseline/x64/liftoff-assembler-x64.h#L4225 And here are my changes: https://chromium-review.googlesource.com/c/v8/v8/+/4742982/1/src/wasm/baseline/x64/liftoff-assembler-x64.h |
|
@sbc100 FYI chrome canary allows memory growth >4gb without any emscripten changes |
|
Spec update with "index" constructor parameter was merged into V8, however I was asked to move memory.init fix to a separate one. So these changes can be merged but tests would still fail until this one gets landed https://chromium-review.googlesource.com/c/v8/v8/+/4775578 |
|
Chrome canary 118.0.5951.0 supports 64bit multi-threading |
This enables quite a few tests that were previously disabled. The extra check for `navigator.userAgent` is because node added support for the navigator object: nodejs/node#47769. This change is also part of #19959, which could land instead.
This enables quite a few tests that were previously disabled. The extra check for `navigator.userAgent` is because node added support for the navigator object: nodejs/node#47769. This change is also part of #19959, which could land instead.
This enables quite a few tests that were previously disabled. The extra check for `navigator.userAgent` is because node added support for the navigator object: nodejs/node#47769. This change is also part of #19959, which could land instead.
|
There is an issue with firefox tests as it does not support creating shared 64bit memory. Should i add @no_firefox to pthreads tests or add no_firefox parameter to @also_with_wasm64? |
dakenf
force-pushed
the
wasm64-pthreads
branch
from
49aa246 to
28a7170
Compare
|
Ok, now i32/i64 was rolled out to canary chrome but node canary lacks behind a bit with |
|
I just update node canary in #20252 |
|
It looks like the changes to the |
Yeah. Just checked and everything was already merged/updated. So this one only enables tests and can be closed |
|
Ah I got mixed up with #20276 |
2 participants
These changes cannot be merged until V8 pull request is approved and merged: https://chromium-review.googlesource.com/c/v8/v8/+/4742982
The PR above allows creating 64bit shared memory without 32bit page limits and fixes memory.fill/init with i64.const dest
I've enabled some tests that pass successfully, others fail because of BigInt conversion failure. Will check them later, once I fix memory.grow above 4gb.