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

src: use RAII to manage the main isolate data #27220

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

This patch encapsulates the main isolate management into a
NodeMainInstance class that manages the resources with RAII
and controls the Isolate::CreateParams (which is necessary
for deserializing snapshots with external references)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot 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. labels Apr 14, 2019
@nodejs-github-bot
Copy link
Collaborator

isolate_ =
NewIsolate(&params, event_loop, per_process::v8_platform.Platform());
CHECK_NOT_NULL(isolate_);
isolate_data_.reset(CreateIsolateData(isolate_,
Copy link
Member Author

Choose a reason for hiding this comment

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

When implementing snapshot this needs to be changed to something like DeserializeIsolateData() which uses GetDataFromSnapshotOnce() (instead of doing e.g. String::NewFromOneByte).

isolate_->GetHeapProfiler()->StartTrackingHeapObjects(true);
}

Local<Context> context = NewContext(isolate_);
Copy link
Member Author

@joyeecheung joyeecheung Apr 14, 2019

Choose a reason for hiding this comment

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

When implementing snapshot this needs to be changed to Context::FromSnapshot() and skips RunBootstrapping() below. The Environment construction would also need to be changed in order to deserialize per-Environment data.

src/node_main_instance.cc Outdated Show resolved Hide resolved
This patch encapsulates the main isolate management into a
NodeMainInstance class that manages the resources with RAII
and controls the Isolate::CreateParams (which is necessary
for deserializing snapshots with external references)
@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 15, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax added a commit to addaleax/node that referenced this pull request Apr 16, 2019
*exit_code = 1;
}

return env;
Copy link
Member

Choose a reason for hiding this comment

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

I know it's existing behavior but *exit_code isn't assigned on the happy path. It works because the caller sets it to zero before calling this method but still...

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an out parameter so at least in my understanding it's fine to not override in the happy path (think of it as overriding an default, whichever the default might be). But I guess with the current implementation resetting it to 0 at the beginning does not hurt either..

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

Landed in cab1dc5

joyeecheung added a commit that referenced this pull request Apr 16, 2019
This patch encapsulates the main isolate management into a
NodeMainInstance class that manages the resources with RAII
and controls the Isolate::CreateParams (which is necessary
for deserializing snapshots with external references)

PR-URL: #27220
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
addaleax added a commit that referenced this pull request Apr 18, 2019
Refs: #27220

PR-URL: #27251
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Nov 1, 2019
This function was not actually available during any part
of the Node 12 release line because it had been removed
earlier (likely accidentally).

Refs: nodejs#27220
addaleax added a commit that referenced this pull request Nov 5, 2019
This function was not actually available during any part
of the Node 12 release line because it had been removed
earlier (likely accidentally).

Refs: #27220

PR-URL: #30098
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
This function was not actually available during any part
of the Node 12 release line because it had been removed
earlier (likely accidentally).

Refs: #27220

PR-URL: #30098
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
targos pushed a commit that referenced this pull request Dec 1, 2019
This function was not actually available during any part
of the Node 12 release line because it had been removed
earlier (likely accidentally).

Refs: #27220

PR-URL: #30098
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
This function was not actually available during any part
of the Node 12 release line because it had been removed
earlier (likely accidentally).

Refs: #27220

PR-URL: #30098
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs 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 / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants