GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — build: enable V8's shared read-only heap by targos · Pull Request #42809 · nodejs/node

Conversation

@targos

@targos targos commented Apr 21, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lin72h, bricss, worstpractice, and okmttdhr reacted with thumbs up emoji lin72h, bricss, and worstpractice reacted with hooray emoji lin72h, bricss, and worstpractice reacted with heart emoji bricss and worstpractice reacted with rocket emoji
All reactions
  • 👍 4 reactions
  • 🎉 3 reactions
  • ❤️ 3 reactions
  • 🚀 2 reactions
@targos
build: enable V8's shared read-only heap
6132527 
It is what V8's build config does by default.
@targos targos added dont-land-on-v12.x labels Apr 21, 2022
@nodejs-github-bot

nodejs-github-bot commented Apr 21, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. 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

targos commented Apr 21, 2022

Copy link
Copy Markdown
Member Author

Refs:

node/deps/v8/BUILD.gn

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" ]
}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Apr 21, 2022

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 21, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 21, 2022
@nodejs-github-bot

nodejs-github-bot commented Apr 21, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Apr 21, 2022

Copy link
Copy Markdown
Member

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.
jasnell and worstpractice reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Apr 21, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Apr 21, 2022

Copy link
Copy Markdown
Member Author

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]
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Apr 21, 2022

Copy link
Copy Markdown
Member

@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)
targos and worstpractice reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Apr 21, 2022

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Apr 22, 2022
CI Reliability 2022-04-22 nodejs/reliability#259
Open
19 tasks
@targos
fixup! build: enable V8's shared read-only heap
0e7cec2
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 22, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 22, 2022
@nodejs-github-bot

nodejs-github-bot commented Apr 22, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Apr 22, 2022
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Apr 22, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot mentioned this pull request Apr 23, 2022
CI Reliability 2022-04-23 nodejs/reliability#260
Open
21 tasks
joyeecheung
joyeecheung approved these changes Apr 23, 2022
View reviewed changes
@targos targos added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Apr 23, 2022
@github-actions github-actions Bot mentioned this pull request Apr 24, 2022
CI Reliability 2022-04-24 nodejs/reliability#261
Open
21 tasks
@targos targos added the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 24, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Apr 24, 2022
@nodejs-github-bot
nodejs-github-bot merged commit daae938 into nodejs:master Apr 24, 2022
@nodejs-github-bot

nodejs-github-bot commented Apr 24, 2022

Copy link
Copy Markdown
Collaborator

Landed in daae938
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos
targos deleted the shared-ro-heap branch April 24, 2022 06:30
@github-actions 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
@targos @xtx1130
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 <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@joyeecheung 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
@targos
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 <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Apr 28, 2022
@joyeecheung
src: make --no-node-snapshot a per-process option
9c60ae2 
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request May 2, 2022
@joyeecheung @targos
src: make --no-node-snapshot a per-process option
37ca110 
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 <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos 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
@codebytere
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
@codebytere
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
@codebytere
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
@codebytere
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
@codebytere
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
@codebytere
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
@codebytere
build: enable V8's shared read-only heap
60e1745 
nodejs/node#42809
codebytere added a commit to electron/electron that referenced this pull request Nov 10, 2022
@codebytere @patchup
chore: upgrade to Node.js v18 (#35999)
75d2caf 
* 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>
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@codebytere @patchup @khalwa
chore: upgrade to Node.js v18 (electron#35999)
b19d67a 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

build Issues and PRs related to Node.js builds or CI infrastructure. commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@targos @nodejs-github-bot @bnoordhuis @joyeecheung @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.