build: enable V8's shared read-only heap

[targos]

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

[nodejs-github-bot]

Review requested:

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

[targos]

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

[targos]

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

[nodejs-github-bot]

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

[bnoordhuis]

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.

[joyeecheung]

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

[targos]

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]

[joyeecheung]

@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)

[joyeecheung]

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.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in daae938