Skip to content

Enable growable array buffers #24684

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jul 10, 2025

Currently this is still behind a flag so we never turn it on by default.

Fixes: #24287

@sbc100 sbc100 requested a review from dschuff July 10, 2025 20:43
@kleisauke
Copy link
Collaborator

IIUC, we could also conditionally exclude these two behind GROWABLE_ARRAYBUFFERS:

#if !defined(EMSCRIPTEN_PURE_WASI)
// Success, update JS (see https://github.com/WebAssembly/WASI/issues/82)
emscripten_notify_memory_growth(0);
#endif

emscripten/src/lib/libcore.js

Lines 2418 to 2424 in 8f12acf

#if PURE_WASI
// In PURE_WASI mode we can't assume the wasm binary was built by emscripten
// and politely notify us on memory growth. Instead we have to check for
// possible memory growth on each syscall.
var pre = '\nif (!HEAPU8.byteLength) _emscripten_notify_memory_growth(0);\n'
library[x + '__deps'].push('emscripten_notify_memory_growth');
#else

@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 11, 2025

IIUC, we could also conditionally exclude these two behind GROWABLE_ARRAYBUFFERS:

#if !defined(EMSCRIPTEN_PURE_WASI)
// Success, update JS (see https://github.com/WebAssembly/WASI/issues/82)
emscripten_notify_memory_growth(0);
#endif

emscripten/src/lib/libcore.js

Lines 2418 to 2424 in 8f12acf

#if PURE_WASI
// In PURE_WASI mode we can't assume the wasm binary was built by emscripten
// and politely notify us on memory growth. Instead we have to check for
// possible memory growth on each syscall.
var pre = '\nif (!HEAPU8.byteLength) _emscripten_notify_memory_growth(0);\n'
library[x + '__deps'].push('emscripten_notify_memory_growth');
#else

Yeah.. I thought of doing that, but didn't want to create yet another library variant. But I think it might be worth it.

@sbc100 sbc100 changed the title Enable growable shared array buffers Enable growable array buffers Jul 11, 2025
@sbc100 sbc100 force-pushed the growable_sab branch 2 times, most recently from c3ba09b to 50c7d06 Compare July 11, 2025 16:38
@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 11, 2025

Done!

@sbc100 sbc100 requested a review from kripken July 11, 2025 16:41
@sbc100 sbc100 force-pushed the growable_sab branch 4 times, most recently from 856b5d2 to 2608cc4 Compare July 11, 2025 18:44
@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 11, 2025

Added a new test

Currently this is still behind a flag so we never turn it on by default.

Fixes: emscripten-core#24287
'proxy': (['-sPROXY_TO_PTHREAD', '-sEXIT_RUNTIME'], 2),
})
def test_pthread_growth_mainthread(self, cflags, pthread_pool_size):
if '-sGROWABLE_ARRAYBUFFERS' in cflags:
self.node_args.append('--experimental-wasm-rab-integration')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth appending to v8_args too, like we do for other experimental features?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@sbc100
Copy link
Collaborator Author

sbc100 commented Jul 12, 2025

This change should be good to go now. PTAL

self.node_args.append('--experimental-wasm-rab-integration')
self.require_node_canary()
else:
self.cflags.append('-Wno-pthreads-mem-growth')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I'd keep this inline in the original flags, after all if that warning shows up in "growable array buffer" mode, we also want the test to error out as it would be unexpected.

Copy link
Collaborator

@RReverser RReverser left a comment

Choose a reason for hiding this comment

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

Left a nitpick, but otherwise very exciting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement support for ResizableArrayBuffer/GrowableSharedArrayBuffer integration
4 participants