Skip to content

tools: refactor snapshot builder - #38902

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:snapshot-refactor
Closed

tools: refactor snapshot builder#38902
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:snapshot-refactor

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Jun 2, 2021

Copy link
Copy Markdown
Member

This patch:

  • Moves the snapshot building code to src/ so that we can reuse it
    later when generating custom snapshots from an entry point accepted
    by the node binary.
  • Create a SnapshotData struct that incorporates all the data useful
    for a snapshot blob, including both the V8 data and the Node.js
    data.

@github-actionsgithub-actionsBot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 Jun 2, 2021
@nodejs-github-bot

nodejs-github-bot commented Jun 2, 2021

Copy link
Copy Markdown
Collaborator

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

bl-ue
bl-ue suggested changes Jun 2, 2021
View reviewed changes

@bl-uebl-ue left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: indexes → indices (while both are correct, the latter is preferred for technical use).

Hopefully I've updated all references to isolate_data_indexes and NodeMainInstance::GetIsolateDataIndices 🤞🏻

Edit: I just realized that you didn't add this code, you moved it from a different file. Scrap these suggestions if they'll cause any more complications than meets the untrained eye viewing this PR ;)

Comment threadsrc/env.h Outdated
Comment threadsrc/node_snapshotable.cc Outdated
Comment threadsrc/node_snapshotable.cc Outdated
@nodejs-github-bot

nodejs-github-bot commented Jun 2, 2021

Copy link
Copy Markdown
Collaborator

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

bl-ue
bl-ue approved these changes Jun 2, 2021
View reviewed changes

@bl-uebl-ue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing: for stringstreams that you only need write to, it's better to use ostringstream.

Comment threadsrc/node_snapshotable.cc Outdated
Comment threadsrc/node_snapshotable.cc Outdated
@RaisinTen

RaisinTen commented Jun 2, 2021

Copy link
Copy Markdown
Member

These need to be updated to GetIsolateDataIndices too:

src/node.cc:1113: indexes = NodeMainInstance::GetIsolateDataIndexes();src/node_main_instance.h:70: static const std::vector<size_t>* GetIsolateDataIndexes();src/node_snapshot_stub.cc:13:const std::vector<size_t>* NodeMainInstance::GetIsolateDataIndexes() {

@bl-ue

bl-ue commented Jun 2, 2021

Copy link
Copy Markdown
Contributor

If you don't want to do all that in this PR, I'd be happy to open a new PR correct the naming, @joyeecheung.

@nodejs-github-bot

nodejs-github-bot commented Jun 2, 2021

Copy link
Copy Markdown
Collaborator

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

bl-ue
bl-ue reviewed Jun 2, 2021
View reviewed changes

@bl-uebl-ue left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the spamming...this is the last one 😛

Comment threadsrc/node.cc Outdated
@joyeecheung
tools: refactor snapshot builder
6cb0e58
This patch:
- Moves the snapshot building code to src/ so that we can reuse it
later when generating custom snapshots from an entry point accepted
by the node binary.
- Create a SnapshotData struct that incorporates all the data useful
for a snapshot blob, including both the V8 data and the Node.js
data.
@nodejs-github-bot

nodejs-github-bot commented Jun 2, 2021

Copy link
Copy Markdown
Collaborator

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

bl-ue
bl-ue reviewed Jun 2, 2021
View reviewed changes
Comment threadsrc/node_snapshotable.cc
@joyeecheungjoyeecheung mentioned this pull request Jun 2, 2021
Closed
@legendecaslegendecas added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 8, 2021
joyeecheung added a commit that referenced this pull request Jun 10, 2021
@joyeecheung
tools: refactor snapshot builder
30e8b5e
This patch:
- Moves the snapshot building code to src/ so that we can reuse it
later when generating custom snapshots from an entry point accepted
by the node binary.
- Create a SnapshotData struct that incorporates all the data useful
for a snapshot blob, including both the V8 data and the Node.js
data.
PR-URL: #38902
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@joyeecheung

joyeecheung commented Jun 10, 2021

Copy link
Copy Markdown
MemberAuthor

Landed in 30e8b5e

targos pushed a commit that referenced this pull request Jun 11, 2021
@joyeecheung@targos
tools: refactor snapshot builder
9bf9ddb
This patch:
- Moves the snapshot building code to src/ so that we can reuse it
later when generating custom snapshots from an entry point accepted
by the node binary.
- Create a SnapshotData struct that incorporates all the data useful
for a snapshot blob, including both the V8 data and the Node.js
data.
PR-URL: #38902
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@danielleadamsdanielleadams mentioned this pull request Jun 14, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jun 17, 2021
@joyeecheung@danielleadams
tools: refactor snapshot builder
68a13a2
This patch:
- Moves the snapshot building code to src/ so that we can reuse it
later when generating custom snapshots from an entry point accepted
by the node binary.
- Create a SnapshotData struct that incorporates all the data useful
for a snapshot blob, including both the V8 data and the Node.js
data.
PR-URL: #38902
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@richardlau

richardlau commented Jul 19, 2021

Copy link
Copy Markdown
Member

Doesn't land cleanly on v14.x-staging. Blocked on at least #37114.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@legendecaslegendecaslegendecas approved these changes
+1 more reviewer
@bl-uebl-uebl-ue approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joyeecheung@nodejs-github-bot@RaisinTen@bl-ue@richardlau@legendecas