FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: cache necessary isolate & context in api/* by XadillaX · Pull Request #38366 · nodejs/node · GitHub

/ node Public

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

Closed
XadillaX wants to merge 1 commit into
nodejs:masterfrom
XadillaX:api/context-isolate
ConversationCommits (1)ChecksFiles changed
Closed

src: cache necessary isolate & context in api/*#38366
XadillaX wants to merge 1 commit into
nodejs:masterfrom
XadillaX:api/context-isolate

Conversation

XadillaX commented Apr 23, 2021

Copy link
Copy Markdown
Contributor

Refs: #37473

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 requested a review from joyeecheung April 23, 2021 12:09
joyeecheung approved these changes Apr 23, 2021
View reviewed changes

joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with a nit

Comment thread src/api/callback.cc
}

HandleScope handle_scope(env_->isolate());
HandleScope handle_scope(isolate);

joyeecheung Apr 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

XadillaX Apr 23, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

/cc @addaleax

addaleax Apr 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 reviewed Apr 23, 2021
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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

Comment thread src/api/callback.cc
}

HandleScope handle_scope(env_->isolate());
HandleScope handle_scope(isolate);

addaleax Apr 23, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Apr 23, 2021

Copy link
Copy Markdown
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 commented Apr 23, 2021

Copy link
Copy Markdown
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 commented Apr 24, 2021

Copy link
Copy Markdown
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 deleted a comment from nodejs-github-bot Apr 29, 2021
nodejs deleted a comment from nodejs-github-bot Apr 29, 2021
nodejs deleted a comment from nodejs-github-bot Apr 29, 2021
nodejs deleted a comment from nodejs-github-bot Apr 29, 2021

nodejs-github-bot commented Apr 29, 2021

Copy link
Copy Markdown
Collaborator

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

XadillaX requested review from Qard and addaleax May 1, 2021 15:52

nodejs-github-bot commented Jun 1, 2021

Copy link
Copy Markdown
Collaborator

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

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

PR-URL: #38366
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

XadillaX commented Jun 1, 2021

Copy link
Copy Markdown
Contributor Author

Landed in f52dc17

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

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

richardlau commented Jul 16, 2021

Copy link
Copy Markdown
Member

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joyeecheung joyeecheung approved these changes
Qard Awaiting requested review from Qard
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

Back | FazBrowse Home | New Git URL