build: fix pointer compression builds #58171
Conversation
- 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.
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.
|
Review requested:
|
nodejs-github-bot
added
build
|
Trying to start a pointer compression build: https://ci.nodejs.org/job/node-test-commit-linux-pointer-compression/784/ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 🚀 New features to boost your workflow:
|
|
CI is happy. Can I get some reviews please? @nodejs/build @nodejs/v8-update |
|
This is so good! |
|
@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. |
github-actions
bot
removed
the
request-ci
build: fix pointer compression builds
be enabled when pointer compression builds are enabled.
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.