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: use per-realm GetBindingData() wherever applicable #49007

Closed
wants to merge 2 commits into from
Closed

src: use per-realm GetBindingData() wherever applicable #49007

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

src: add per-realm GetBindingData() method

This version avoids the additional access to the embedder slot
when we already have a reference to the realm.

src: use per-realm GetBindingData() wherever applicable

This reduce the number of embedder slot accesses and also removes
the assumption in a few binding methods that the current realm is
the principal realm of the current environment (which is not true
for shadow realms).

Refs: #48836

@joyeecheung
src: add per-realm GetBindingData() method
267a4ab 
This version avoids the additional access to the embedder slot
when we already have a reference to the realm.
@joyeecheung
src: use per-realm GetBindingData() wherever applicable
96c05b4 
This reduce the number of embedder slot accesses and also removes
the assumption in a few binding methods that the current realm is
the principal realm of the current environment (which is not true
for shadow realms).
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net
  • @nodejs/realm
  • @nodejs/startup
  • @nodejs/url

@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 Aug 3, 2023
legendecas
legendecas approved these changes Aug 3, 2023
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thank you for handling this.

legendecas

This comment was marked as duplicate.

Sign in to view

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Aug 4, 2023
Open
25 tasks
mcollina
mcollina approved these changes Aug 4, 2023
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions bot mentioned this pull request Aug 11, 2023
Open
29 tasks
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Aug 15, 2023
Open
22 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Aug 16, 2023
Open
24 tasks
@legendecas legendecas added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Aug 16, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 16, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6f9d6f2...6391b3b

nodejs-github-bot pushed a commit that referenced this pull request Aug 16, 2023
@joyeecheung @nodejs-github-bot
src: add per-realm GetBindingData() method
3578bee 
This version avoids the additional access to the embedder slot
when we already have a reference to the realm.

PR-URL: #49007
Refs: #48836
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Aug 16, 2023
@joyeecheung @nodejs-github-bot
src: use per-realm GetBindingData() wherever applicable
6391b3b 
This reduce the number of embedder slot accesses and also removes
the assumption in a few binding methods that the current realm is
the principal realm of the current environment (which is not true
for shadow realms).

PR-URL: #49007
Refs: #48836
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
This was referenced Aug 17, 2023
Open
Open
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@joyeecheung @UlisesGascon
src: add per-realm GetBindingData() method
184bbdd 
This version avoids the additional access to the embedder slot
when we already have a reference to the realm.

PR-URL: #49007
Refs: #48836
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@joyeecheung @UlisesGascon
src: use per-realm GetBindingData() wherever applicable
2a35383 
This reduce the number of embedder slot accesses and also removes
the assumption in a few binding methods that the current realm is
the principal realm of the current environment (which is not true
for shadow realms).

PR-URL: #49007
Refs: #48836
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@Qard Qard Qard approved these changes

@anonrig anonrig anonrig approved these changes

@legendecas legendecas legendecas approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. 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.
7 participants
@joyeecheung @nodejs-github-bot @mcollina @Qard @anonrig @legendecas @RafaelGSS