Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: speeding up Node.js startup using V8 snapshot #17058

Closed
hashseed opened this issue Nov 15, 2017 · 79 comments
Closed

RFC: speeding up Node.js startup using V8 snapshot #17058

hashseed opened this issue Nov 15, 2017 · 79 comments
Labels
discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.

Comments

@hashseed
Copy link
Member

I recently went through Node.js bootstrapping code, and think that we could make it a lot faster using V8 snapshot. I wrote a design doc that captures the main points.

This is somewhat separate from the discussion on using V8 snapshot to capture arbitrary initialized state, discussed here. The main difference is that the set of native modules is known upfront, and there is no ambiguity about the native bindings that need to be known to V8's serializer/deserializer.

I'm doing this as sort of a side project, so it may take some time for me to make progress. Any help is welcome.

@hashseed hashseed added discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. v8 engine Issues and PRs related to the V8 dependency. labels Nov 15, 2017
@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Nov 15, 2017
@nodejs nodejs deleted a comment from rehack Nov 17, 2017
@mhdawson
Copy link
Member

Sounds great. Is there a repo/branch people can take a look at the changes you have so far for this work ?

@hashseed
Copy link
Member Author

I have a development branch here. All tests still pass, because they are not affected. Running node snapshot attempts to create a snapshot blob. It currently still fails, but I have collected almost all native references. There is still a long way to go though.

@juancampa
Copy link

juancampa commented Nov 18, 2017

Hi @hashseed, this is amazing and very needed, thanks for working on this. I asked on #17103 but it seems pertinent to this thread (since it's an RFC) so I'll ask here as well.

Would these changes allow for taking snapshots in arbitrary points in time? as opposed to right after startup? If not, do you mind explaining if such a thing would even be feasible. Seems like the guys from Chakra have been working on something like it for Time Travel Debugging (as mentioned by @mrkmarron: #13877 (comment))

To elaborate a little, I'm thinking something like CRIU, where you can send a signal to the process (e.g. kill -USR2 <pid> or some other mechanism) and have the process serialize it's state (to a predefined location maybe) and optionally exit. Then later in time you could restore that serialized state by providing it to a new node process (e.g. node --restore <state-file>)

@hashseed
Copy link
Member Author

It would, once finished, help achieve that goal. Current V8 API is not laid out to serialize arbitrary isolates at arbitrary times, but something in that direction would be thinkable.

However, there are quite a few C++ objects allocated by native modules. Each module would have to provide a way to serialize/deserialize its objects, and we would need a way to dispatch to each module for serialization/deserialization. This hurdle applies to Node with Chakra too.

Another issue is that there is no good way to serialize/deserialize the stack, including C++ frames and local variables that hold onto objects on the JS heap.

@mrkmarron
Copy link

Hi @hashseed, I was able to take a quick look at the document and the commits. As you say it looks like there is a lot of work left but this looks like a great start. Please let me know if there is anything that might be useful to help and I will try to make some time. Otherwise, I'll definitely keep an eye on the progress here.

Thanks for this effort!

@hashseed
Copy link
Member Author

Hi Mark,

this is more of a solve-issues-as-we-go approach. Once I have a roughly working version done, there will be some more careful auditing necessary to see which part of initialization needs to be done after snapshotting because they are runtime-dependent, for example gated by command line args. Maybe that's something you can dig into, if you have the time?

@mrkmarron
Copy link

Sure, I have looked into this type of thing in the past. The two most common approaches seem to be either (1) making this type of startup a pure programmer responsibility or (2) using symbolic execution (ala prepack). Neither solution is 100% satisfactory but I'll try to spend some time investigating to get a better understanding of things.

@hashseed
Copy link
Member Author

hashseed commented Nov 20, 2017

I updated the branch. So far running node snapshot creates a snapshot_blob.bin file into the working directory. With the file in place, running node uses the snapshot, deserializes an isolate and a context, and then immediately quits (because I haven't spent time on making the deserialized context work yet :D).

For some reason I wasn't able to reproduce the 400ms start up for vanilla Node.js anymore, but rather 200ms. I wonder whether I made some mistake with the build. But when I instrumented my branch with uv_hrtime() I was at least able to reproduce 4x speed up.

I'll carry on to make the deserialized context actually work.

@hashseed
Copy link
Member Author

@addaleax @bnoordhuis @TimothyGu is there any reason we store the environment in a v8::External object to pass to function templates as data, instead of getting the current context from the isolate and the environment from there?

v8::Externals are tricky to serialize. They should belong to the context. Function and object templates belong to the isolate, but can own v8::External objects as data. With the current API there is no good way serialize templates as part of the context.

v8::External objects are also kinda hacky. They look like JavaScript objects, but are special in that they are context-independent and don't have a constructor.

@bnoordhuis
Copy link
Member

is there any reason we store the environment in a v8::External object to pass to function templates as data, instead of getting the current context from the isolate and the environment from there?

@hashseed See commit 7e88a93, it's explained in the commit log.

@hashseed
Copy link
Member Author

I see. So these functions are fundamentally bound to the context. Would it help if FunctionCallbackInfo can return the function's creation context?

@bnoordhuis
Copy link
Member

Yep, that should work if it's also done for PropertyCallbackInfo.

@hashseed
Copy link
Member Author

Quite a few yaks to shave... I think I'll just pile things on this branch first and later disentangle into single PRs.

@hashseed
Copy link
Member Author

hashseed commented Nov 20, 2017

@bnoordhuis for PropertyCallbackInfo the Holder should suffice? I don't think there is a way to move an interceptor from one object to another.

As for FunctionCallbackInfo, the corresponding class in v8::internal, FunctionCallbackArguments, actually already has a callee argument in the constructor. It's just never used or stored anywhere. That can be fixed.

Edit: looks like removing callee was intentional. Guess I'll solve it differently: FunctionTemplates and ObjectTemplates that reference some data that is context-dependent (including v8::External) must be serialized as part of the context.

@hashseed
Copy link
Member Author

@bnoordhuis I just verified with @verwaest that inside a function template callback, the current context is indeed already the context of the function being called.

That means we don't need to wrap the environment in a v8::External anymore.

@verwaest-zz
Copy link

This isn't true yet for PropertyCallbackInfo and Interceptors though. We could do the same there as we do for "lazy accessor pairs" (function template based accessors).

@liqyan
Copy link
Contributor

liqyan commented Dec 21, 2017

Hi hashseed, i have an experimental implemention here. It roughly works.

Just like points you wrote in design doc, i do:

  • splitting up node_bootstrap.js's startup into two functions: bootstrap and new startup, for node_mksnapshot, bootstrap is the entry, for node booting from snapshot, the latter is the entry.
  • registering external references to SnapshotCreator, All is c++ function, and node::Environment*

and some more:

  • support registered Persistent/Global handles serialization
  • distinguish Persistent/Global handles from Context aware or not, registering the former to Context, registering the latter to Isolate
  • while booting from snapshot, first allocate memory for node::Environment without initialization, register it to Isolate, then using placement new to create node::Environment after Context has been created
  • change the behavior of external string serialization. External string will still be external string if it's ExternalStringResource is registered. v8 SnapshotCreator converts external string to internal string. node's builtin modules are all exteranl strings, after booting from snapshot, they are internal strings whose chars allocated in v8 heap. These internal strings can't be shared if node fork/exec or just fork child processes

some remain issues:

  • external references registering and c++ function binding looks like redundantly
  • setupGlobalConsole consumes significant time ( 14ms, vs total running time 39ms with snapshot on my workstation), but node_mksnapshot can't run with it currently(crashed), node booting from snapshot must restore the status below:
    • one ChannelWrap and one SignalWrap object will be created
    • two TTYWrap object will be created, and some uv tty operations will be executed

Running the command time ./node -e "", with snapshot ther result is 39ms, without is 88ms, on my workstation.

howto build

 
./configure --with-node-snapshot
make
cd out/Release
./node_mksnapshot --startup-blob=snapshot_blob.bin

Hope this helps.

@hashseed
Copy link
Member Author

hashseed commented Dec 21, 2017

Wow. That's a big surprise. I'll definitely check out your implementation once I have some time.

I must admit that I haven't actually pursued my prototype much further in the last couple of weeks since I haven't had time and had to wrap up a few other projects at the end of the year.

When you say "external references registering and c++ function binding looks like redundantly" do you mean it's not necessary? Is that because you do not set up any function templates when creating the snapshot? Function templates has been one of the biggest blockers for me since they point to the Environment via data pointer, which makes them no longer context-independent.

Cutting the startup time to less than half is a bit less than what I expected, but definitely a significant win! We should try to get this merged, maybe incrementally.

@liqyan
Copy link
Contributor

liqyan commented Dec 22, 2017

@hashseed

When you say "external references registering and c++ function binding looks like redundantly" do you mean it's not necessary?

What i want to say is c++ functions binding in node builtin addon initialization procedure, and registering these c++ functions to SnapshotCreator while creating snapshot or to Isolate while booting from snapshot, do similar things, like env->SetMethod(target, name, api_callback), or external_references[idx]=api_callback. If there's a way to do these things in one mechanism, it'd be great.

Cutting the startup time to less than half is a bit less than what I expected, but definitely a significant win! We should try to get this merged, maybe incrementally.

As i said, setupGlobalConsole consumes significant time. Once node_mksnapshot can be executed with it, the startup time will be 1/4.

@hashseed
Copy link
Member Author

I opened an issue on V8 to track implementing serializer/deserializer support for external strings: https://bugs.chromium.org/p/v8/issues/detail?id=7240

I have some other ideas than the one you implemented. I'll work on that and land that upstream.

@hashseed
Copy link
Member Author

hashseed commented Dec 22, 2017

I took a look at the changes that affect V8:

  1. deps: v8: serialize/deserialize external string table correctly
  2. deps: v8: support ser/des global/eternal handles
  3. deps: v8: support visit external string

To support external strings, I came up with this upstream change. Does that solve the issue that commits (1) and (3) solve? I'm not entirely sure what commit (1) fixes.

As for commit (2), I think it goes into the right direction, but is a bit over-engineered. You are putting global and eternal handles into fixed arrays before serialization, so that you can re-create global and external handles after deserialization. You distinguish between ones that are context-independent and ones that are context-dependent. A few thoughts on that:

  • With this approach, do we really need to distinguish between global and external handles, if they are both stored in fixed arrays?
  • Should context-dependent eternal handles even exist in the first place?
  • Assuming that we have no context-dependent eternal handles, we could deserialize eternal handles into a std::vector of pointers that we explicitly visit during GC, like the partial-snapshot-cache. Getting external handles from that std::vector would be simple, as that's essentially the same data structure the eternal handle implementation already uses.

I would love if you could contribute (2) to upstream V8. Due to legal reasons I can't just copy your code and submit it upstream.

What i want to say is c++ functions binding in node builtin addon initialization procedure, and registering these c++ functions to SnapshotCreator while creating snapshot or to Isolate while booting from snapshot, do similar things, like env->SetMethod(target, name, api_callback), or external_references[idx]=api_callback. If there's a way to do these things in one mechanism, it'd be great.

Do you mean to say that you would like to have a way to add external references late? How late? At the point of SnapshotCreator::CreateBlob? That should be doable and simply an API change.

joyeecheung added a commit that referenced this issue Jun 2, 2020
Original commit message:

    [snapshot] Do not defer ArrayBuffers during snapshotting

    ArrayBuffer instances are serialized by first re-assigning a index
    to the backing store field, then serializing the object, and then
    storing the actual backing store address again (and the same for the
    ArrayBufferExtension). If serialization of the object itself is deferred,
    the real backing store address is written into the snapshot, which cannot be
    processed when deserializing, leading to a crash.

    This fixes this by not deferring ArrayBuffer serialization and adding a DCHECK
    for the crash that previously occurred.

    Change-Id: Id9bea8268061bd0770cde7bfeb6695248978f994
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2144123
    Commit-Queue: Jakob Gruber <[email protected]>
    Reviewed-by: Dan Elphick <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67114}

Refs: v8/v8@ea0719b

PR-URL: #33300
Refs: v8/v8@bb9f0c2
Refs: v8/v8@22014de
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
joyeecheung added a commit that referenced this issue Jun 2, 2020
Original commit message:

    [snapshot] Improve snapshot docs and error printing

    - Minor improvements to the documentation for snapshotting.
    - Add newlines to printed errors where necessary.

    Change-Id: I822e7e850adb67eae73b51c23cf34e40ba3106f0
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2144954
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67111}

Refs: v8/v8@bb9f0c2

PR-URL: #33300
Refs: v8/v8@ea0719b
Refs: v8/v8@22014de
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
joyeecheung added a commit that referenced this issue Jun 2, 2020
Original commit message:

    Reland "[snapshot] rehash JSMap and JSSet during deserialization"

    This is a reland of 8374feed55a5b3010f2e9593560a2d84f9f6725f.

    Fixed rehashing of global proxy keys by creating its identity hash
    early, before the deserialization of the context snapshot.

    Original change's description:
    > [snapshot] rehash JSMap and JSSet during deserialization
    >
    > To rehash JSMap and JSSet, we simply replace the backing store
    > with a new one created with the new hash.
    >
    > Bug: v8:9187
    > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983
    > Commit-Queue: Joyee Cheung <[email protected]>
    > Reviewed-by: Jakob Gruber <[email protected]>
    > Reviewed-by: Camillo Bruni <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#67663}

    Bug: v8:9187, v8:10523
    Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67999}

Refs: v8/v8@22014de

PR-URL: #33300
Refs: v8/v8@ea0719b
Refs: v8/v8@bb9f0c2
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this issue Jun 18, 2020
Original commit message:

    [snapshot] Do not defer ArrayBuffers during snapshotting

    ArrayBuffer instances are serialized by first re-assigning a index
    to the backing store field, then serializing the object, and then
    storing the actual backing store address again (and the same for the
    ArrayBufferExtension). If serialization of the object itself is deferred,
    the real backing store address is written into the snapshot, which cannot be
    processed when deserializing, leading to a crash.

    This fixes this by not deferring ArrayBuffer serialization and adding a DCHECK
    for the crash that previously occurred.

    Change-Id: Id9bea8268061bd0770cde7bfeb6695248978f994
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2144123
    Commit-Queue: Jakob Gruber <[email protected]>
    Reviewed-by: Dan Elphick <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67114}

Refs: v8/v8@ea0719b

PR-URL: #33300
Refs: v8/v8@bb9f0c2
Refs: v8/v8@22014de
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this issue Jun 18, 2020
Original commit message:

    [snapshot] Improve snapshot docs and error printing

    - Minor improvements to the documentation for snapshotting.
    - Add newlines to printed errors where necessary.

    Change-Id: I822e7e850adb67eae73b51c23cf34e40ba3106f0
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2144954
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67111}

Refs: v8/v8@bb9f0c2

PR-URL: #33300
Refs: v8/v8@ea0719b
Refs: v8/v8@22014de
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this issue Jun 18, 2020
Original commit message:

    Reland "[snapshot] rehash JSMap and JSSet during deserialization"

    This is a reland of 8374feed55a5b3010f2e9593560a2d84f9f6725f.

    Fixed rehashing of global proxy keys by creating its identity hash
    early, before the deserialization of the context snapshot.

    Original change's description:
    > [snapshot] rehash JSMap and JSSet during deserialization
    >
    > To rehash JSMap and JSSet, we simply replace the backing store
    > with a new one created with the new hash.
    >
    > Bug: v8:9187
    > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983
    > Commit-Queue: Joyee Cheung <[email protected]>
    > Reviewed-by: Jakob Gruber <[email protected]>
    > Reviewed-by: Camillo Bruni <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#67663}

    Bug: v8:9187, v8:10523
    Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67999}

Refs: v8/v8@22014de

PR-URL: #33300
Refs: v8/v8@ea0719b
Refs: v8/v8@bb9f0c2
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this issue Jun 30, 2020
Original commit message:

    [snapshot] Do not defer ArrayBuffers during snapshotting

    ArrayBuffer instances are serialized by first re-assigning a index
    to the backing store field, then serializing the object, and then
    storing the actual backing store address again (and the same for the
    ArrayBufferExtension). If serialization of the object itself is deferred,
    the real backing store address is written into the snapshot, which cannot be
    processed when deserializing, leading to a crash.

    This fixes this by not deferring ArrayBuffer serialization and adding a DCHECK
    for the crash that previously occurred.

    Change-Id: Id9bea8268061bd0770cde7bfeb6695248978f994
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2144123
    Commit-Queue: Jakob Gruber <[email protected]>
    Reviewed-by: Dan Elphick <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67114}

Refs: v8/v8@ea0719b

PR-URL: #33300
Refs: v8/v8@bb9f0c2
Refs: v8/v8@22014de
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this issue Jun 30, 2020
Original commit message:

    [snapshot] Improve snapshot docs and error printing

    - Minor improvements to the documentation for snapshotting.
    - Add newlines to printed errors where necessary.

    Change-Id: I822e7e850adb67eae73b51c23cf34e40ba3106f0
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2144954
    Reviewed-by: Toon Verwaest <[email protected]>
    Commit-Queue: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67111}

Refs: v8/v8@bb9f0c2

PR-URL: #33300
Refs: v8/v8@ea0719b
Refs: v8/v8@22014de
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
codebytere pushed a commit that referenced this issue Jun 30, 2020
Original commit message:

    Reland "[snapshot] rehash JSMap and JSSet during deserialization"

    This is a reland of 8374feed55a5b3010f2e9593560a2d84f9f6725f.

    Fixed rehashing of global proxy keys by creating its identity hash
    early, before the deserialization of the context snapshot.

    Original change's description:
    > [snapshot] rehash JSMap and JSSet during deserialization
    >
    > To rehash JSMap and JSSet, we simply replace the backing store
    > with a new one created with the new hash.
    >
    > Bug: v8:9187
    > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983
    > Commit-Queue: Joyee Cheung <[email protected]>
    > Reviewed-by: Jakob Gruber <[email protected]>
    > Reviewed-by: Camillo Bruni <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#67663}

    Bug: v8:9187, v8:10523
    Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67999}

Refs: v8/v8@22014de

PR-URL: #33300
Refs: v8/v8@ea0719b
Refs: v8/v8@bb9f0c2
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this issue Jul 4, 2020
Original commit message:

    Reland "[snapshot] rehash JSMap and JSSet during deserialization"

    This is a reland of 8374feed55a5b3010f2e9593560a2d84f9f6725f.

    Fixed rehashing of global proxy keys by creating its identity hash
    early, before the deserialization of the context snapshot.

    Original change's description:
    > [snapshot] rehash JSMap and JSSet during deserialization
    >
    > To rehash JSMap and JSSet, we simply replace the backing store
    > with a new one created with the new hash.
    >
    > Bug: v8:9187
    > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983
    > Commit-Queue: Joyee Cheung <[email protected]>
    > Reviewed-by: Jakob Gruber <[email protected]>
    > Reviewed-by: Camillo Bruni <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#67663}

    Bug: v8:9187, v8:10523
    Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67999}

Refs: v8/v8@22014de

PR-URL: nodejs#33300
Refs: v8/v8@ea0719b
Refs: v8/v8@bb9f0c2
Refs: nodejs#17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit to targos/node that referenced this issue Jul 13, 2020
Original commit message:

    Reland "[snapshot] rehash JSMap and JSSet during deserialization"

    This is a reland of 8374feed55a5b3010f2e9593560a2d84f9f6725f.

    Fixed rehashing of global proxy keys by creating its identity hash
    early, before the deserialization of the context snapshot.

    Original change's description:
    > [snapshot] rehash JSMap and JSSet during deserialization
    >
    > To rehash JSMap and JSSet, we simply replace the backing store
    > with a new one created with the new hash.
    >
    > Bug: v8:9187
    > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983
    > Commit-Queue: Joyee Cheung <[email protected]>
    > Reviewed-by: Jakob Gruber <[email protected]>
    > Reviewed-by: Camillo Bruni <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#67663}

    Bug: v8:9187, v8:10523
    Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67999}

Refs: v8/v8@22014de

PR-URL: nodejs#33300
Refs: v8/v8@ea0719b
Refs: v8/v8@bb9f0c2
Refs: nodejs#17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Jul 13, 2020
Original commit message:

    Reland "[snapshot] rehash JSMap and JSSet during deserialization"

    This is a reland of 8374feed55a5b3010f2e9593560a2d84f9f6725f.

    Fixed rehashing of global proxy keys by creating its identity hash
    early, before the deserialization of the context snapshot.

    Original change's description:
    > [snapshot] rehash JSMap and JSSet during deserialization
    >
    > To rehash JSMap and JSSet, we simply replace the backing store
    > with a new one created with the new hash.
    >
    > Bug: v8:9187
    > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983
    > Commit-Queue: Joyee Cheung <[email protected]>
    > Reviewed-by: Jakob Gruber <[email protected]>
    > Reviewed-by: Camillo Bruni <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#67663}

    Bug: v8:9187, v8:10523
    Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67999}

Refs: v8/v8@22014de

PR-URL: #33300
Refs: v8/v8@ea0719b
Refs: v8/v8@bb9f0c2
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit to targos/node that referenced this issue Jul 16, 2020
Original commit message:

    Reland "[snapshot] rehash JSMap and JSSet during deserialization"

    This is a reland of 8374feed55a5b3010f2e9593560a2d84f9f6725f.

    Fixed rehashing of global proxy keys by creating its identity hash
    early, before the deserialization of the context snapshot.

    Original change's description:
    > [snapshot] rehash JSMap and JSSet during deserialization
    >
    > To rehash JSMap and JSSet, we simply replace the backing store
    > with a new one created with the new hash.
    >
    > Bug: v8:9187
    > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983
    > Commit-Queue: Joyee Cheung <[email protected]>
    > Reviewed-by: Jakob Gruber <[email protected]>
    > Reviewed-by: Camillo Bruni <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#67663}

    Bug: v8:9187, v8:10523
    Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67999}

Refs: v8/v8@22014de

PR-URL: nodejs#33300
Refs: v8/v8@ea0719b
Refs: v8/v8@bb9f0c2
Refs: nodejs#17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 16, 2020
Original commit message:

    Reland "[snapshot] rehash JSMap and JSSet during deserialization"

    This is a reland of 8374feed55a5b3010f2e9593560a2d84f9f6725f.

    Fixed rehashing of global proxy keys by creating its identity hash
    early, before the deserialization of the context snapshot.

    Original change's description:
    > [snapshot] rehash JSMap and JSSet during deserialization
    >
    > To rehash JSMap and JSSet, we simply replace the backing store
    > with a new one created with the new hash.
    >
    > Bug: v8:9187
    > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983
    > Commit-Queue: Joyee Cheung <[email protected]>
    > Reviewed-by: Jakob Gruber <[email protected]>
    > Reviewed-by: Camillo Bruni <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#67663}

    Bug: v8:9187, v8:10523
    Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67999}

Refs: v8/v8@22014de

Backport-PR-URL: #34356
PR-URL: #33300
Refs: v8/v8@ea0719b
Refs: v8/v8@bb9f0c2
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 16, 2020
Original commit message:

    Reland "[snapshot] rehash JSMap and JSSet during deserialization"

    This is a reland of 8374feed55a5b3010f2e9593560a2d84f9f6725f.

    Fixed rehashing of global proxy keys by creating its identity hash
    early, before the deserialization of the context snapshot.

    Original change's description:
    > [snapshot] rehash JSMap and JSSet during deserialization
    >
    > To rehash JSMap and JSSet, we simply replace the backing store
    > with a new one created with the new hash.
    >
    > Bug: v8:9187
    > Change-Id: I90c25b18b33b7bc2b6ffe1b89fe17aa5f978b517
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2143983
    > Commit-Queue: Joyee Cheung <[email protected]>
    > Reviewed-by: Jakob Gruber <[email protected]>
    > Reviewed-by: Camillo Bruni <[email protected]>
    > Cr-Commit-Position: refs/heads/master@{#67663}

    Bug: v8:9187, v8:10523
    Change-Id: I7a0319b1d10ff07644de902fec43e7c2b1dd8da9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2212085
    Reviewed-by: Leszek Swirski <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Reviewed-by: Jakob Gruber <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#67999}

Refs: v8/v8@22014de

Backport-PR-URL: #34356
PR-URL: #33300
Refs: v8/v8@ea0719b
Refs: v8/v8@bb9f0c2
Refs: #17058
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@joyeecheung
Copy link
Member

Since the core bootstrap is now in the builtin snapshot, and we are working towards including more of the bootstrap into the snapshot, maybe it's time to close this and open a new tracking issue?

@gengjiawen
Copy link
Member

Since the core bootstrap is now in the builtin snapshot, and we are working towards including more of the bootstrap into the snapshot, maybe it's time to close this and open a new tracking issue?

LGTM.

@joyeecheung
Copy link
Member

Opened #35711 for discussions around future snapshot work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests