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

bootstrap: make snapshot reproducible #50983

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Nov 30, 2023

deps: V8: cherry-pick b639938e99fa

Original commit message:

[snapshot] support context embedder data serialization

Previously all context embedder data are serialized verbatim,
so if a context embedder data slot contains a pointer, embedders
need to figure out how to store data for it on the side for the
snapshot. They also have to reset the pointers to null to make
the snapshot reproducible.

To address this issue this patch adds
v8::(Des|S)erializeContextDataCallback as part
of v8::(Des|S)erializeEmbedderFieldsCallback and use them during
(de)serialization of context embedder data. The bahavior is similar
to that of v8::(Des|S)erializeInternalFieldsCallback: If the
returned data is not empty, the returned data is going to be
written into the snapshot blob and passed to the embedder during
deserialization. Otherwise the data is serialized verbatim.

Change-Id: I4113c76fc6dc8e3503cde5533320402454e62097
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5074252
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/main@{#92675}

Refs: v8/v8@b639938

deps: V8: cherry-pick 7fc698c9039d

Original commit message:

[snapshot] allow NativeContext serialization to be deferred

To serialize the context embedder data, we just need the back
reference to the embedder data array. The context can be deferred.

Bug: v8:14662
Change-Id: I6a3c9f35596cd17b7e6bc773cf598922b5b7488d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5367362
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#92892}

Refs: v8/v8@7fc698c

src: add utilities to help debugging reproducibility of snapshots

  • Print offsets in blob serializer
  • Add a special node:generate_default_snapshot ID to generate
    the built-in snapshot.
  • Improve logging

bootstrap: make snapshot reproducible

This patch uses the new V8 API to {de}serialize context slots for
snapshot in order to make the snapshot reproducible. Also
added a test for the reproducibility of snapshots.

Refs: nodejs/build#3043

@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 30, 2023
@haraldh
Copy link

haraldh commented Jan 31, 2024

great work! keep it up!

Original commit message:

    [snapshot] support context embedder data serialization

    Previously all context embedder data are serialized verbatim,
    so if a context embedder data slot contains a pointer, embedders
    need to figure out how to store data for it on the side for the
    snapshot. They also have to reset the pointers to null to make
    the snapshot reproducible.

    To address this issue this patch adds
    v8::(Des|S)erializeContextDataCallback as part
    of v8::(Des|S)erializeEmbedderFieldsCallback and use them during
    (de)serialization of context embedder data. The bahavior is similar
    to that of v8::(Des|S)erializeInternalFieldsCallback: If the
    returned data is not empty, the returned data is going to be
    written into the snapshot blob and passed to the embedder during
    deserialization. Otherwise the data is serialized verbatim.

    Change-Id: I4113c76fc6dc8e3503cde5533320402454e62097
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5074252
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Leszek Swirski <leszeks@chromium.org>
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Cr-Commit-Position: refs/heads/main@{#92675}

Refs: v8/v8@b639938
Original commit message:

    [snapshot] allow NativeContext serialization to be deferred

    To serialize the context embedder data, we just need the back
    reference to the embedder data array. The context can be deferred.

    Bug: v8:14662
    Change-Id: I6a3c9f35596cd17b7e6bc773cf598922b5b7488d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5367362
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Leszek Swirski <leszeks@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#92892}

Refs: v8/v8@7fc698c
@joyeecheung joyeecheung changed the title WIP: reproducible snapshot bootstrap: make snapshot reproducible Mar 22, 2024
@joyeecheung joyeecheung marked this pull request as ready for review March 22, 2024 19:08
@joyeecheung joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Mar 22, 2024
- Print offsets in blob serializer
- Add a special node:generate_default_snapshot ID to generate
  the built-in snapshot.
- Improve logging
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 22, 2024
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

Somehow this is reproducible locally (macOS + x64 & arm64), and in the CI, on FreeBSD and SmartOS, but not on others. There are still 3-4 words of differences in the snapshot on those platforms. I'll investigate.

This patch uses the new V8 API to {de}serialize context slots for
snapshot in order to make the snapshot reproducible. Also
added a test for the reproducibility of snapshots.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants