Repository navigation
node_mksnapshot.cc: code cache generated based on temporary RO heap layout #47636
Description
Version
No response
Platform
No response
Subsystem
No response
What steps will reproduce the bug?
No response
How often does it reproduce? Is there a required condition?
No response
What is the expected behavior? Why is that the expected behavior?
No response
What do you see instead?
fbe1478 moved code cache generation into node_mksnapshot, but I suspect this process is (and has always been) unsupported by V8. It has just worked somewhat by accident til now.
A hidden requirement of V8's code serializer is that it runs inside an Isolate that has been deserialized from the final snapshot, i.e. the same snapshot that is used in the node binary itself. Why? Because SerializeReadOnlyObjectReference encodes the RO object ref as {chunk index, offset}, but RO space layout can and does change between RO space serialization- and deserialization-time (and thus the serialized encoded RO ref may be invalid).
I'm hitting this locally (based on a V8 change I'm currently working on), note how the offset of the "load" string changes:
in node_mksnapshot: 0x0a78926543d9 <String[4]: #load> offset 82904
in node 0x32bf83c14789 <String[4]: #load> offset 83848.
This results in the code-deserializer creating a pointer pointing at a random location inside RO space.
We could fix this in several ways,
- change the snapshot-generation process in Node s.t. code serialization happens after heap serialization,
- disable
node_use_node_code_cache - (long-term vision) change V8 to keep RO space layout fixed, or
- (pragmatic hack) have Node pass a parameter to ScriptCompiler::CompileFunction to enable slow-but-safe RO object serialization (although I hesitate to expose this quirk through the API)
@joyeecheung wdyt?
Additional information
No response
Activity
joyeecheung commentedon Apr 20, 2023
Thanks for reporting this, I did fbe1478 because of the same issue but I didn't realize that the RO heap could also change after serialization. I think what I was seeing from https://bugs.chromium.org/p/v8/issues/detail?id=12718 was that the code cache should be serialized from the same heap where the snapshot was taken to match the RO heap layout, but the order didn't matter as much.
Option 1 sounds good enough for me for now, thanks (3 would be better in the long term, or maybe some cleverer way to encode the RO heap positions can be invented to avoid requiring the layout to be fixed). Is it safe to still use the serialized isolate for compiling code? If so I think maybe simply moving
node/src/node_snapshotable.cc
Line 1213 in 5c86f22
CreateBlob()would be enough? (I tried it in this branch, which seems to work fine with the V8 on main branch, but I'm not sure if it works with your patch)schuay commentedon Apr 25, 2023
Re option 1: no, unfortunately it's not enough to move code-cache serialization til after CreateBlob. It has to use precisely the same initial heap shape as 'production' isolates, i.e. an Isolate deserialized from the final snapshot. The simplest solution is to do code-cache serialization in a separate binary.
Any opinions on how we should proceed on the V8 side until this is fixed? I would like to land my change (crrev.com/c/4440686) soon, but am currently blocked by the node failure at https://logs.chromium.org/logs/node-ci/buildbucket/cr-buildbucket/8783294649412924721/+/u/build/compile/stdout. Suggestion: disable
node_use_node_code_cachein our build until your fix trickles down.joyeecheung commentedon Apr 25, 2023
To clarify, is it safe to deserialize that snapshot in the same process later and use the deserialized isolate to generate code cache? Or does it have to be deserialized from a new process? Does turning off shared RO heap make a difference here?
I am concerned that more differences in build configs e.g. disabling code cache in V8's build would result in more breakages that go unnoticed until it gets fixed properly, it depends on how long it's going to take to fix it though.
joyeecheung commentedon Apr 25, 2023
Or, from what I can tell from https://chromium-review.googlesource.com/c/v8/v8/+/4440686, it seems possible to make the timing of the sealing of the RO space configurable in V8 internally? If V8 keeps the old timing of sealing the RO heap for the embedder API, and uses the new extended timing for the internal default snapshot, we could have some time to tackle it without turning off code cache in Node.js while the V8 default snapshot gets the benefit. We still need to figure out how to address the potential mismatch of the code cache, but has always been the case anyway, and having the V8 default snapshot built somewhat differently from the snapshots built using the embedder API seems reasonable to me. Setting aside the code cache mismatch issue, I can see that maybe for the default V8 snapshot, the more moved from the isolate snapshot to the RO snapshot, the better, but that's not necessarily the case for snapshots built from the embedder API.
schuay commentedon Apr 26, 2023
With some custom logic (which would fully tear down RO space prior to deserialization) that'd work, but not out of the box. Currently we only fully support this in a new process.
In V8, I've moved the
Sealcall around a bit to include initial Context setup as well. But I think that's unrelated to this issue - the RO space layout changes between the serialize- and deserialize steps due to the way we currently visit heap objects for serialization.Also note RO space currently isn't unsealed when building custom snapshots. The steps that that led to the crash I've been seeing were:
joyeecheung commentedon Apr 26, 2023
I think for Node.js that might be an easier way out if we can still do it in the same process, or otherwise we need to update node.gyp to allow another circular dependency in the current circular dependency (libnode -> mkcodecache -> libnode + mksnapshot ->libnode), which isn't necessarily impossible, but would be more complicated to implement.
schuay commentedon Aug 2, 2023
We fixed this on the V8 side by preserving RO space layout through serialization-deserialization.
Temporarily disable node_use_node_code_cache
Revert "Temporarily disable node_use_node_code_cache"
bootstrap: build code cache from deserialized isolate
bootstrap: build code cache from deserialized isolate