Skip to content

src: apply getCallSite optimization - #55174

Closed
RafaelGSS wants to merge 4 commits into
nodejs:mainfrom
RafaelGSS:optimize-getcallsite
Closed

src: apply getCallSite optimization#55174
RafaelGSS wants to merge 4 commits into
nodejs:mainfrom
RafaelGSS:optimize-getcallsite

Conversation

@RafaelGSS

@RafaelGSSRafaelGSS commented Sep 30, 2024

Copy link
Copy Markdown
Member

Applying one of @jasnell suggestions #54380 (comment)

Result an possible performance boost of ~14%

➜ node (optimize-getcallsite) node-benchmark-compare compare.csv ✭ ✱
confidence improvement accuracy (*) (**) (***)
util/get-callsite.js method='CPP' n=100 *** 14.00 % ±3.69% ±4.92% ±6.43%

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Sep 30, 2024
@avivkelleravivkeller added the needs-benchmark-ci PR that need a benchmark CI run. label Sep 30, 2024
@RafaelGSS

RafaelGSS commented Sep 30, 2024

Copy link
Copy Markdown
MemberAuthor

I believe the coverage errors are unrelated to this PR. Are we tracking it somewhere? @nodejs/build

@targos

targos commented Sep 30, 2024

Copy link
Copy Markdown
Member

It appeared with #53256. Unrelated to build WG activities.

joyeecheung
joyeecheung reviewed Sep 30, 2024
View reviewed changes
Comment threadsrc/node_util.cc Outdated
anonrig
anonrig reviewed Sep 30, 2024
View reviewed changes
Comment threadsrc/node_util.cc Outdated
static void GetCallSite(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Isolate* isolate = env->isolate();
v8::HandleScope handle_scope(isolate);

@anonriganonrigSep 30, 2024

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.

Q for learning purposes: Why do we need a handle scope here?

@RafaelGSSRafaelGSSSep 30, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Oh this is no longer necessary. I was investigating a segmentation fault and though the variables were gcCollected before going back to JS.

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 30, 2024
@nodejs-github-bot

nodejs-github-bot commented Sep 30, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Sep 30, 2024•
edited by RafaelGSS
Loading

Copy link
Copy Markdown
Collaborator

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

@RafaelGSS

RafaelGSS commented Oct 1, 2024

Copy link
Copy Markdown
MemberAuthor

Is it ok if I go ahead and land this PR? Coverage should be solved on main, but rebasing would require a new CI run.

@RafaelGSSRafaelGSS added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 2, 2024
RafaelGSS added a commit that referenced this pull request Oct 2, 2024
@RafaelGSS
src: apply getCallSite optimization
18536d9
PR-URL: #55174
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@RafaelGSS

RafaelGSS commented Oct 2, 2024

Copy link
Copy Markdown
MemberAuthor

Landed in 18536d9

targos pushed a commit that referenced this pull request Oct 4, 2024
@RafaelGSS@targos
src: apply getCallSite optimization
6db7170
PR-URL: #55174
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2024
@RafaelGSS@targos
src: apply getCallSite optimization
b1d47d0
PR-URL: #55174
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@aduh95aduh95 mentioned this pull request Oct 9, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anonriganonriganonrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
@legendecaslegendecaslegendecas approved these changes
@jasnelljasnellAwaiting requested review from jasnell

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-benchmark-ciPR that need a benchmark CI run.needs-ciPRs that need a full CI run.utilIssues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@RafaelGSS@targos@nodejs-github-bot@anonrig@joyeecheung@legendecas@marco-ippolito@avivkeller