src: do not enable wasm trap handler if there's not enough vmem#62132
src: do not enable wasm trap handler if there's not enough vmem#62132joyeecheung wants to merge 3 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62132 +/- ##
==========================================
+ Coverage 89.66% 89.67% +0.01%
==========================================
Files 676 676
Lines 206550 206710 +160
Branches 39546 39579 +33
==========================================
+ Hits 185198 185366 +168
- Misses 13478 13500 +22
+ Partials 7874 7844 -30
🚀 New features to boost your workflow:
|
When the system does not have enough virtual memory for the wasm cage, installing the trap handler would cause any code allocating wasm memory to throw. Therefore it's useful for the embedder to know when the system doesn't have enough virtual address space to allocate enough wasm cages and in that case, skip the trap handler installation so that wasm code can at least work (even not at the maximal performance). Node.js previously has a command line option --disable-wasm-trap-handler to fully disable trap-based bound checks, this new API would allow it to adapt automatically while keeping the optimization in the happy path, since it's not always possible for end users to opt-into disabling trap-based bound checks (for example, when a VS Code Server is loaded in a remote server for debugging). Refs: nodejs/node#62132 Refs: microsoft/vscode#251777 Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233 Reviewed-by: Jakob Kummerow <jkummerow@chromium.org> Reviewed-by: Michael Lippautz <mlippautz@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Reviewed-by: Clemens Backes <clemensb@chromium.org> Cr-Commit-Position: refs/heads/main@{#105702}
Original commit message:
[api] Add V8::GetWasmMemoryReservationSizeInBytes()
When the system does not have enough virtual memory for the wasm
cage, installing the trap handler would cause any code allocating
wasm memory to throw. Therefore it's useful for the embedder to
know when the system doesn't have enough virtual address space
to allocate enough wasm cages and in that case, skip the
trap handler installation so that wasm code can at least work
(even not at the maximal performance).
Node.js previously has a command line option
--disable-wasm-trap-handler to fully disable trap-based bound checks,
this new API would allow it to adapt automatically while keeping the
optimization in the happy path, since it's not always possible for
end users to opt-into disabling trap-based bound checks (for example,
when a VS Code Server is loaded in a remote server for debugging).
Refs: nodejs#62132
Refs: microsoft/vscode#251777
Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#105702}
Refs: v8/v8@bef0d9c
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
0c431d9 to
831617f
Compare
Original commit message:
[api] Add V8::GetWasmMemoryReservationSizeInBytes()
When the system does not have enough virtual memory for the wasm
cage, installing the trap handler would cause any code allocating
wasm memory to throw. Therefore it's useful for the embedder to
know when the system doesn't have enough virtual address space
to allocate enough wasm cages and in that case, skip the
trap handler installation so that wasm code can at least work
(even not at the maximal performance).
Node.js previously has a command line option
--disable-wasm-trap-handler to fully disable trap-based bound checks,
this new API would allow it to adapt automatically while keeping the
optimization in the happy path, since it's not always possible for
end users to opt-into disabling trap-based bound checks (for example,
when a VS Code Server is loaded in a remote server for debugging).
Refs: nodejs#62132
Refs: microsoft/vscode#251777
Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#105702}
Refs: v8/v8@bef0d9c
Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
de51d96 to
37b5c46
Compare
|
cc @nodejs/cpp-reviewers @nodejs/v8 PTAL, this should address microsoft/vscode#251777 (there are also a bunch of similarly confused users on the Internet affected by this issue, like https://www.reddit.com/r/vscode/comments/1ld6fp7/downloading_vs_code_server_loop_when_connecting/ and https://stackoverflow.com/questions/65055955/vscode-ssh-extension-stuck-on-the-installation-step) |
|
Also, we should update |
37b5c46 to
f61ab3b
Compare
|
@Aditi-1400 Updated the docs, can you take another look? Thanks |
f61ab3b to
bab88bb
Compare
bab88bb to
6219aca
Compare
| test-wasm-allocation: SKIP | ||
| test-wasm-allocation-auto-adapt: SKIP | ||
| test-wasm-allocation-memory64: SKIP | ||
| test-wasm-allocation-memory64-auto-adapt: SKIP |
There was a problem hiding this comment.
This test file wasn't added, also we should probably add test-wasm-allocation-disable-trap-handler here?
There was a problem hiding this comment.
Right, I merged that test into test-wasm-allocation-auto-adapt; removed, thanks for catching!
As for test-wasm-allocation-disable-trap-handler - there's probably no need to skip, it should pass everywhere regardless (it just allocates a bunch of wasm memory - this can also serve to test that the option is harmless even on platforms where trap handler is not supported in the first place).
|
q (non-blocking): What even is "not enough vmem" on 64-bit systems? On which setups can this be hit except for manually setting ulimit on vmem? (and e.g. very old OpenVZ bean counting) Does this mean that we should start caring about VSZ? |
RISC-V 64bit cpus with sv39 only allocates 256GiB of address space for userspace.
We are hitting it very often on riscv64 with sv39. e.g. #60591 and riscv-forks/electron#3 It is happening even for an operation simple as |
| // enabled. | ||
| return true; | ||
| } | ||
| uint64_t virtual_memory_available = static_cast<uint64_t>(lim.rlim_cur); |
There was a problem hiding this comment.
(Non blocking)
The virtual memory available to the process is usually much more than the virtual memory available for usage in wasm trap handler.
e.g. In nodejs/doc-kit#691 (comment) , I observed that only a single wasm memory is successfully allocated, while the available virtual address space is 256GiB (riscv64 with sv39). It's very likely that address space fragmentation caused it.
Maybe it would be better to check if the available virtual memory could contain more cages rather than just one cage. But that being said, I am not sure where to draw the line.
There was a problem hiding this comment.
That would help accommodate more use cases yeah, though I guess for the initial version we can just go with RLIMIT_AS for now for the more common "someone is setting it somewhere to be < 16GB" case - which means even core functionality can be incomplete (since llhttp requires at least 1 cage). I think it'll probably be unlikely for 64-bit architectures to have a natural limit that is smaller than that. For space sizes in-between, this is going to be tricky, you'll only see issues from multiple user-land WASM, and that's harder to accomondate just from the Node.js side - you'll likely need something in V8 instead.
From what I tell - likely some legacy operational stuff that sets ulimit -v something (in my case that was why I introduced the
I don't think this is common. Most of the time Node.js allocates virtual memory as needed, and don't reserve huge chunk of address space that it doesn't use. Trap-based bound checks in WASM is special in that it reserves a large chunk of address space (currently 8-16GB) even if it just needs to commit 64KB. |
The first commit is a backport of https://chromium-review.googlesource.com/c/v8/v8/+/7638233
This complements
--disable-wasm-trap-handlerand allows users to keep wasm functional in an environment with virtual memory restrictions (e.g. in the case of VS code server run for remote access, the user does not necessary always have the permission on the server to configureNODE_OPTIONSor--disable-wasm-trap-handler- in fact they don't even necessarily know this has anything to do with WASM on Node.js since they shouldn't be aware of what VS code server is implemented with).Supersedes #60788 - this keeps the estimation logic in Node.js instead, and also keeps it in POSIX because on other systems the issue that this is solving is not practical (e.g. on Windows, one cannot bound the virtual memory available without bounding the physical memory, and it does not make a difference whether trap based bound checks are enabled or not when physical memory limit is reached)
We can tweak the threshold of estimation of needed wasm cages in 1 in Node.js to reduce the possibility of 2, this PR currently sets the threshold to 1, since even within Node.js core there's already undici that would use one cage whenever
fetch()i sused.Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233