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.

Sign up for GitHub

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

Jump to bottom

src: cache necessary isolate & context in api/* #38366

Closed
wants to merge 1 commit into from
Closed

src: cache necessary isolate & context in api/* #38366

wants to merge 1 commit into from

Conversation

XadillaX
Copy link
Contributor

Refs: #37473

@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. needs-ci PRs that need a full CI run. labels Apr 23, 2021
@XadillaX XadillaX requested a review from joyeecheung April 23, 2021 12:09
joyeecheung
joyeecheung approved these changes Apr 23, 2021
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

src/api/callback.cc
@@ -130,7 +134,7 @@ void InternalCallbackScope::Close() {
return;
}

HandleScope handle_scope(env_->isolate());
HandleScope handle_scope(isolate);
Copy link
Member

Choose a reason for hiding this comment

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

hm, I know this exists before this PR, but why do we create a handle scope here? May be it should just be deleted, AFAICT this is meant to be invoked when there is already a HandleScope

Copy link
Contributor Author

@XadillaX XadillaX Apr 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

/cc @addaleax

Copy link
Member

Choose a reason for hiding this comment

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

I mean, just because there’s an outer handle scope doesn’t mean that it’s pointless to have an inner one, e.g. when InternalCallbackScope is used in a loop

addaleax
addaleax reviewed Apr 23, 2021
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

@XadillaX The code LGTM, but neither this nor the PR it references have any explanation why we would want to do this…? For contexts I can see the argument that eventually we might have multi-context readiness, but for the Isolate pointer this seems like a fairly pointless change

src/api/callback.cc
@@ -130,7 +134,7 @@ void InternalCallbackScope::Close() {
return;
}

HandleScope handle_scope(env_->isolate());
HandleScope handle_scope(isolate);
Copy link
Member

Choose a reason for hiding this comment

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

I mean, just because there’s an outer handle scope doesn’t mean that it’s pointless to have an inner one, e.g. when InternalCallbackScope is used in a loop

@XadillaX
Copy link
Contributor Author

@XadillaX The code LGTM, but neither this nor the PR it references have any explanation why we would want to do this…? For contexts I can see the argument that eventually we might have multi-context readiness, but for the Isolate pointer this seems like a fairly pointless change

Yeah, the context changing is for avoiding multi-times PersistentToLocal. But Isolate is just a handy thing. Shall I change isolate back?

@addaleax
Copy link
Member

Yeah, the context changing is for avoiding multi-times PersistentToLocal.

If you’re saying that this has a performance impact: We use PersistentToLocal::Strong for env->context(), which is essentially just a pointer lookup. This patch is not going to have any significant performance benefits, so it really makes just sense to focus on what’s cleanest from a code readability point of view.

I’m not really minding the code change, but I do think that the project should a) have a common code style for these questions and b) enforce that through a linter, otherwise changes will just happen back and forth forever (#38172 literally did the reverse of this only a week ago).

@XadillaX
Copy link
Contributor Author

Yeah, the context changing is for avoiding multi-times PersistentToLocal.

If you’re saying that this has a performance impact: We use PersistentToLocal::Strong for env->context(), which is essentially just a pointer lookup. This patch is not going to have any significant performance benefits, so it really makes just sense to focus on what’s cleanest from a code readability point of view.

I’m not really minding the code change, but I do think that the project should a) have a common code style for these questions and b) enforce that through a linter, otherwise changes will just happen back and forth forever (#38172 literally did the reverse of this only a week ago).

I see. I think we may create an issue about both a and b but not in this one.

Shall we change the CPPLINT rule?

@nodejs nodejs deleted a comment from nodejs-github-bot Apr 29, 2021
@nodejs nodejs deleted a comment from nodejs-github-bot Apr 29, 2021
@nodejs nodejs deleted a comment from nodejs-github-bot Apr 29, 2021
@nodejs nodejs deleted a comment from nodejs-github-bot Apr 29, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@XadillaX XadillaX requested review from Qard and addaleax May 1, 2021 15:52
@nodejs-github-bot
Copy link
Collaborator

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

XadillaX added a commit that referenced this pull request Jun 1, 2021
@XadillaX
src: cache necessary isolate & context in api/*
f52dc17 
Refs: #37473

PR-URL: #38366
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@XadillaX
Copy link
Contributor Author

XadillaX commented Jun 1, 2021

Landed in f52dc17

@XadillaX XadillaX closed this Jun 1, 2021
danielleadams pushed a commit that referenced this pull request Jun 2, 2021
@XadillaX @danielleadams
src: cache necessary isolate & context in api/*
120849f 
Refs: #37473

PR-URL: #38366
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@danielleadams danielleadams mentioned this pull request Jun 2, 2021
Merged
@richardlau
Copy link
Member

This doesn't land cleanly on v14.x-staging.

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

@joyeecheung joyeecheung joyeecheung approved these changes

@Qard Qard Awaiting requested review from Qard

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@XadillaX @addaleax @nodejs-github-bot @richardlau @joyeecheung