build: enable V8's shared read-only heap by targos · Pull Request #42809 · nodejs/node · GitHub

It is what V8's build config does by default.

Review requested:

@nodejs/gyp
@nodejs/v8-update

Refs:

Lines 488 to 491 in fe85cf7

    
           if (v8_enable_shared_ro_heap == "") { 
        
             v8_enable_shared_ro_heap = !v8_enable_pointer_compression || 
        
                                        v8_enable_pointer_compression_shared_cage 
        
           }

node/deps/v8/BUILD.gn

Lines 933 to 935 in fe85cf7

    
           if (v8_enable_shared_ro_heap) { 
        
             defines += [ "V8_SHARED_RO_HEAP" ] 
        
           }

@nodejs/v8 is this something that we want to do? What would it improve for Node.js?

CI: https://ci.nodejs.org/job/node-test-pull-request/43612/

What would it improve for Node.js?

In theory the shared RO heap speeds up isolate creation and sending objects between them but I don't think it does anything until you start linking isolates together with Isolate::CreateParams::experimental_attach_to_shared_isolate.

I think it's worth enabling even just to avoid having another configuration not covered by the V8 CI.

This breaks a test in debug builds: https://ci.nodejs.org/job/node-test-commit-arm-debug/1703/nodes=ubuntu1804-arm64/testReport/junit/(root)/test/parallel_test_worker_nearheaplimit_deadlock/

#
# Fatal error in ../deps/v8/src/heap/read-only-spaces.cc, line 69
# Check failed: read_only_blob_checksum_ == snapshot_checksum (<unprintable> vs. 3337505632).
#
#
#
#FailureMessage Object: 0xffff8cb44588
 1: 0xaaaacb36e4e8 node::DumpBacktrace(_IO_FILE*) [out/Debug/node]
 2: 0xaaaacb52a254  [out/Debug/node]
 3: 0xaaaacb52a280  [out/Debug/node]
 4: 0xaaaaccddd5a4 V8_Fatal(char const*, int, char const*, ...) [out/Debug/node]
 5: 0xaaaacbb46ac4 v8::internal::ReadOnlyArtifacts::VerifyChecksum(v8::internal::SnapshotData*, bool) [out/Debug/node]
 6: 0xaaaacbb4319c v8::internal::ReadOnlyHeap::SetUp(v8::internal::Isolate*, v8::internal::SnapshotData*, bool) [out/Debug/node]
 7: 0xaaaacb9ddb74 v8::internal::Isolate::Init(v8::internal::SnapshotData*, v8::internal::SnapshotData*, v8::internal::SnapshotData*, bool) [out/Debug/node]
 8: 0xaaaacc0c0c0c  [out/Debug/node]
 9: 0xaaaacb7281b8 v8::Isolate::Initialize(v8::Isolate*, v8::Isolate::CreateParams const&) [out/Debug/node]
10: 0xaaaacb59e0f0 node::worker::WorkerThreadData::WorkerThreadData(node::worker::Worker*) [out/Debug/node]
11: 0xaaaacb597b10 node::worker::Worker::Run() [out/Debug/node]
12: 0xaaaacb599adc  [out/Debug/node]
13: 0xaaaacb599b70  [out/Debug/node]
14: 0xffff8f75b088  [/lib/aarch64-linux-gnu/libpthread.so.0]

@targos That's because the test is intentionally using the default snapshot instead of the Node.js one to make sure that the OOM does not occur during deserialization..so basically enabling shared RO heap breaks --no-node-snapshot in workers (if the main thread is using the embedded snapshot), but since --no-node-snapshot is a internal debug flag that's probably not a big deal. I think adding // Flags: --no-node-snapshot at the top of the test should fix the crash (because then both the main thread and the worker thread will be using the default snapshot)

Another more solid solution would be to disallow specifying --no-node-snapshot in workers and make sure that they always inherit whatever the main thread uses.

CI: https://ci.nodejs.org/job/node-test-pull-request/43632/

CI: https://ci.nodejs.org/job/node-test-pull-request/43639/

Landed in daae938

It is what V8's build config does by default. PR-URL: nodejs#42809 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

It is what V8's build config does by default. PR-URL: #42809 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

We enable the shared read-only heap which currently requires that the snapshot used in different isolates in the same process to be the same. Therefore --no-node-snapshot is a per-process option. PR-URL: #42864 Refs: #42809 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs/node#42809

* chore: update to Node.js v18 * child_process: improve argument validation nodejs/node#41305 * bootstrap: support configure-time user-land snapshot nodejs/node#42466 * chore: update GN patch * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * src: use a typed array internally for process._exiting nodejs/node#43883 * chore: lib/internal/bootstrap -> lib/internal/process * src: disambiguate terms used to refer to builtins and addons nodejs/node#44135 * chore: remove redudant browserGlobals patch * chore: update BoringSSL patch * src: allow embedder-provided PageAllocator in NodePlatform nodejs/node#38362 * chore: fixup Node.js crypto tests - nodejs/node#44171 - nodejs/node#41600 * lib: add Promise methods to avoid-prototype-pollution lint rule nodejs/node#43849 * deps: update V8 to 10.1 nodejs/node#42657 * src: add kNoBrowserGlobals flag for Environment nodejs/node#40532 * chore: consolidate asar initialization patches * deps: update V8 to 10.1 nodejs/node#42657 * deps: update V8 to 9.8 nodejs/node#41610 * src,crypto: remove AllocatedBuffers from crypto_spkac nodejs/node#40752 * build: enable V8's shared read-only heap nodejs/node#42809 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 * chore: fixup ESM patch * chore: fixup patch indices * src: merge NativeModuleEnv into NativeModuleLoader nodejs/node#43824 * [API] Pass OOMDetails to OOMErrorCallback https://chromium-review.googlesource.com/c/v8/v8/+/3647827 * src: iwyu in cleanup_queue.cc * src: return Maybe from a couple of functions nodejs/node#39603 * src: clean up embedder API nodejs/node#35897 * src: refactor DH groups to delete crypto_groups.h nodejs/node#43896 * deps,src: use SIMD for normal base64 encoding nodejs/node#39775 * chore: remove deleted source file * chore: update patches * chore: remove deleted source file * lib: add fetch nodejs/node#41749 * chore: remove nonexistent node specs * test: split report OOM tests nodejs/node#44389 * src: trace fs async api nodejs/node#44057 * http: trace http request / response nodejs/node#44102 * test: split test-crypto-dh.js nodejs/node#40451 * crypto: introduce X509Certificate API nodejs/node#36804 * src: split property helpers from node::Environment nodejs/node#44056 * nodejs/node#38905 bootstrap: implement run-time user-land snapshots via --build-snapshot and --snapshot-blob * lib,src: implement WebAssembly Web API nodejs/node#42701 * fixup! deps,src: use SIMD for normal base64 encoding * fixup! src: refactor DH groups to delete crypto_groups.h * chore: fixup base64 GN file * fix: check that node::InitializeContext() returns true * chore: delete _noBrowserGlobals usage * chore: disable fetch in renderer procceses * dns: default to verbatim=true in dns.lookup() nodejs/node#39987 Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

build: enable V8's shared read-only heap

6132527

It is what V8's build config does by default.

targos added dont-land-on-v12.x labels Apr 21, 2022

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Apr 21, 2022

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 21, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 21, 2022

github-actions Bot mentioned this pull request Apr 22, 2022

CI Reliability 2022-04-22 nodejs/reliability#259

Open

19 tasks

fixup! build: enable V8's shared read-only heap

0e7cec2

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2022

jasnell approved these changes Apr 22, 2022

View reviewed changes

github-actions Bot mentioned this pull request Apr 23, 2022

CI Reliability 2022-04-23 nodejs/reliability#260

Open

21 tasks

joyeecheung approved these changes Apr 23, 2022

View reviewed changes

targos added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 23, 2022

github-actions Bot mentioned this pull request Apr 24, 2022

CI Reliability 2022-04-24 nodejs/reliability#261

Open

21 tasks

targos added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 24, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 24, 2022

nodejs-github-bot merged commit daae938 into nodejs:master Apr 24, 2022

targos deleted the shared-ro-heap branch April 24, 2022 06:30

github-actions Bot mentioned this pull request Apr 25, 2022

CI Reliability 2022-04-25 nodejs/reliability#262

Open

16 tasks

xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022

build: enable V8's shared read-only heap

e80c56c

It is what V8's build config does by default. PR-URL: nodejs#42809 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

joyeecheung mentioned this pull request Apr 25, 2022

src: make --no-node-snapshot a per-process option #42864

Merged

This was referenced Apr 26, 2022

CI Reliability 2022-04-26 nodejs/reliability#263

Open

CI Reliability 2022-04-27 nodejs/reliability#264

Open

CI Reliability 2022-04-28 nodejs/reliability#265

Open

targos added a commit that referenced this pull request Apr 28, 2022

build: enable V8's shared read-only heap

4050b0d

It is what V8's build config does by default. PR-URL: #42809 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

targos mentioned this pull request May 2, 2022

v18.1.0 release proposal #42943

Merged

codebytere added a commit to electron/electron that referenced this pull request Oct 12, 2022

build: enable V8's shared read-only heap

62f1995

nodejs/node#42809

codebytere added a commit to electron/electron that referenced this pull request Oct 17, 2022

build: enable V8's shared read-only heap

35f59c5

nodejs/node#42809

codebytere added a commit to electron/electron that referenced this pull request Oct 19, 2022

build: enable V8's shared read-only heap

050dd90

nodejs/node#42809

codebytere added a commit to electron/electron that referenced this pull request Oct 19, 2022

build: enable V8's shared read-only heap

407014b

nodejs/node#42809

codebytere added a commit to electron/electron that referenced this pull request Oct 24, 2022

build: enable V8's shared read-only heap

7f7903d

nodejs/node#42809

codebytere added a commit to electron/electron that referenced this pull request Nov 8, 2022

build: enable V8's shared read-only heap

cc6a3b7

nodejs/node#42809

codebytere added a commit to electron/electron that referenced this pull request Nov 8, 2022

build: enable V8's shared read-only heap

60e1745

nodejs/node#42809

build: enable V8's shared read-only heap by targos · Pull Request #42809 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: enable V8's shared read-only heap - #42809

build: enable V8's shared read-only heap#42809
nodejs-github-bot merged 2 commits into
nodejs:masterfrom
targos:shared-ro-heap

targos commented Apr 21, 2022

Uh oh!

nodejs-github-bot commented Apr 21, 2022

Uh oh!

targos commented Apr 21, 2022

Uh oh!

targos commented Apr 21, 2022

Uh oh!

nodejs-github-bot commented Apr 21, 2022

Uh oh!

bnoordhuis commented Apr 21, 2022

Uh oh!

joyeecheung commented Apr 21, 2022

Uh oh!

targos commented Apr 21, 2022

Uh oh!

joyeecheung commented Apr 21, 2022

Uh oh!

joyeecheung commented Apr 21, 2022

Uh oh!

nodejs-github-bot commented Apr 22, 2022

Uh oh!

nodejs-github-bot commented Apr 22, 2022

Uh oh!

nodejs-github-bot commented Apr 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

build: enable V8's shared read-only heap by targos · Pull Request #42809 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: enable V8's shared read-only heap - #42809

build: enable V8's shared read-only heap#42809nodejs-github-bot merged 2 commits intonodejs:masternodejs/node:masterfrom targos:shared-ro-heaptargos/node:shared-ro-heapCopy head branch name to clipboard

Conversation

targos commented Apr 21, 2022

Uh oh!

nodejs-github-bot commented Apr 21, 2022

Uh oh!

targos commented Apr 21, 2022

Uh oh!

targos commented Apr 21, 2022

Uh oh!

nodejs-github-bot commented Apr 21, 2022

Uh oh!

bnoordhuis commented Apr 21, 2022

Uh oh!

joyeecheung commented Apr 21, 2022

Uh oh!

targos commented Apr 21, 2022

Uh oh!

joyeecheung commented Apr 21, 2022

Uh oh!

joyeecheung commented Apr 21, 2022

Uh oh!

nodejs-github-bot commented Apr 22, 2022

Uh oh!

nodejs-github-bot commented Apr 22, 2022

Uh oh!

nodejs-github-bot commented Apr 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

build: enable V8's shared read-only heap#42809
nodejs-github-bot merged 2 commits into
nodejs:masterfrom
targos:shared-ro-heap