nodejs
build: fix pointer compression builds
build: fix pointer compression builds
- Remove usage of deprecated V8::InitializeSandbox().
- External code space and pointer compression shared cage must be enabled when pointer compression builds are enabled.
- We cannot enable the sandbox because that requires allocating the array buffer backing stores in the sandbox - we currently have many backing stores tied to pointers from C++ land that are not even necessarily dynamic (e.g. in static storage). Until we manage to get rid of all those, sandbox cannot be enabled. Note that enabling pointer compression without enabling sandbox is unsupported by V8, and can be broken at any time.
test: skip wasm-allocation tests for pointer compression builds
V8 isolate group initialization forces allocation of the virtual memory cage with pointer compression builds and simply would not work when there is a smaller hard limit on the virtual memory.
Trying to start a pointer compression build: https://ci.nodejs.org/job/node-test-commit-linux-pointer-compression/784/
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 90.14%. Comparing base (
5fb879c) to head (a3fab01). Report is 134 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #58171 +/- ##
========================================
Coverage 90.13% 90.14%
========================================
Files 630 630
Lines 186611 186754 +143
Branches 36631 36652 +21
========================================
+ Hits 168204 168351 +147
- Misses 11192 11193 +1
+ Partials 7215 7210 -5
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
CI is happy. Can I get some reviews please? @nodejs/build @nodejs/v8-update
@joyeecheung Have you checked if there is any memory leak with this approach? We have tried a similar approach to this PR, and it caused a memory leak. Enabling pointer compression without enabling sandbox, as the V8 does not support it, would cause this leak.
I didn't check anything other than whether the tests pass, since my goal is just to make the CI green again, even if there is a leak it's better than being broken completely and make the daily master CI red every day, and if there's a bug the fix likely needs to be in the upstream anyway, and isn't caught by our tests so it would just be like many other known issues we have even with the regular build, waiting for other volunteers to fix.
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6557/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6557/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6557/
...Enabling pointer compression without enabling sandbox, as the V8 does not support it, would cause this leak.
In workerd we run with a configuration that enables pointer compression without the sandbox and have for some time. Yes, there are times when we see issues with v8 updates but they are rare but I don't believe we've seen leaks. I think ideally we want to work towards enabling the sandbox but there are lots of fixups we need to do first, so I think starting here and working towards that goal incrementally is best.
Commit Queue failed
- Loading data for nodejs/node/pull/58171 ✔ Done loading data for nodejs/node/pull/58171 ----------------------------------- PR info ------------------------------------ Title build: fix pointer compression builds (#58171) Author Joyee Cheung <[email protected]> (@joyeecheung) Branch joyeecheung:fix-pointer-compression -> nodejs:main Labels build, v8 engine, test, tools, author ready, needs-ci Commits 2 - build: fix pointer compression builds - test: skip wasm-allocation tests for pointer compression builds Committers 1 - Joyee Cheung <[email protected]> PR-URL: https://github.com/nodejs/node/pull/58171 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: James M Snell <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/58171 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: James M Snell <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Sun, 04 May 2025 19:47:28 GMT ✔ Approvals: 3 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/58171#pullrequestreview-2817969673 ✔ - Juan José Arboleda (@juanarbol): https://github.com/nodejs/node/pull/58171#pullrequestreview-2827093221 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/58171#pullrequestreview-2831915105 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-05-18T07:41:05Z: https://ci.nodejs.org/job/node-test-pull-request/66878/ - Querying data for job/node-test-pull-request/66878/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 58171 From https://github.com/nodejs/node * branch refs/pull/58171/merge -> FETCH_HEAD ✔ Fetched commits as 9eb9c26b261e..a3fab01641b8 -------------------------------------------------------------------------------- Auto-merging common.gypi Auto-merging configure.py Auto-merging tools/v8_gypfiles/features.gypi [main 46f4cc58f9] build: fix pointer compression builds Author: Joyee Cheung <[email protected]> Date: Sun May 4 20:58:06 2025 +0200 4 files changed, 22 insertions(+), 4 deletions(-) Auto-merging tools/test.py [main 78e79d96e7] test: skip wasm-allocation tests for pointer compression builds Author: Joyee Cheung <[email protected]> Date: Sun May 4 21:38:34 2025 +0200 2 files changed, 5 insertions(+), 1 deletion(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- build: fix pointer compression builds
- Remove usage of deprecated V8::InitializeSandbox().
- External code space and pointer compression shared cage must be enabled when pointer compression builds are enabled.
- We cannot enable the sandbox because that requires allocating the array buffer backing stores in the sandbox - we currently have many backing stores tied to pointers from C++ land that are not even necessarily dynamic (e.g. in static storage). Until we manage to get rid of all those, sandbox cannot be enabled. Note that enabling pointer compression without enabling sandbox is unsupported by V8, and can be broken at any time.
PR-URL: https://github.com/nodejs/node/pull/58171 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: James M Snell <[email protected]>
[detached HEAD 44a98bc64d] build: fix pointer compression builds Author: Joyee Cheung <[email protected]> Date: Sun May 4 20:58:06 2025 +0200 4 files changed, 22 insertions(+), 4 deletions(-) Rebasing (3/4) Rebasing (4/4) Executing: git node land --amend --yes --------------------------------- New Message ---------------------------------- test: skip wasm-allocation tests for pointer compression builds
V8 isolate group initialization forces allocation of the virtual memory cage with pointer compression builds and simply would not work when there is a smaller hard limit on the virtual memory.
PR-URL: https://github.com/nodejs/node/pull/58171 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: James M Snell <[email protected]>
[detached HEAD ef93c6f910] test: skip wasm-allocation tests for pointer compression builds Author: Joyee Cheung <[email protected]> Date: Sun May 4 21:38:34 2025 +0200 2 files changed, 5 insertions(+), 1 deletion(-) Successfully rebased and updated refs/heads/main.
ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.https://github.com/nodejs/node/actions/runs/15097514259
Landed in db2aae8022d50bd6546274031ff95b12fc347358...20c4b80ffca2738046b21f5792e239ccfa8c9321
