-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: speed up process.getActiveResourcesInfo()
#46014
src: speed up process.getActiveResourcesInfo()
#46014
Conversation
This change reduces the number of calls that were crossing the JS-C++ boundary to 1 and also removes the need for calling Array::New() multiple times internally and ArrayPrototypeConcat-ing the results later on, thus improving performance. Refs: nodejs#44445 (review) Signed-off-by: Darshan Sen <raisinten@gmail.com>
Review requested:
|
Benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1280
|
Benchmark CI(timers): https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1281 Results
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Environment* env = Environment::GetCurrent(args); | ||
std::vector<Local<Value>> resources_info; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you can omit using std::vector
for better performance in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestions on how you think that can be done?
Refs: nodejs#46014 (comment) Signed-off-by: Darshan Sen <raisinten@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
Commit Queue failed- Loading data for nodejs/node/pull/46014 ✔ Done loading data for nodejs/node/pull/46014 ----------------------------------- PR info ------------------------------------ Title src: speed up `process.getActiveResourcesInfo()` (#46014) Author Darshan Sen (@RaisinTen) Branch RaisinTen:src/speed-up-process.getActiveResourcesInfo -> nodejs:main Labels c++, lib / src, author ready, needs-ci, commit-queue-squash Commits 3 - src: speed up process.getActiveResourcesInfo() - lib: explain timeoutInfo - lib: update comment Committers 2 - Darshan Sen - GitHub PR-URL: https://github.com/nodejs/node/pull/46014 Reviewed-By: Antoine du Hamel Reviewed-By: Chengzhong Wu ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46014 Reviewed-By: Antoine du Hamel Reviewed-By: Chengzhong Wu -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - lib: update comment ℹ This PR was created on Thu, 29 Dec 2022 14:11:08 GMT ✔ Approvals: 2 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/46014#pullrequestreview-1232745904 ✔ - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/46014#pullrequestreview-1233540022 ✔ Last GitHub CI successful ℹ Last Benchmark CI on 2022-12-29T20:19:51Z: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1281 ℹ Last Full PR CI on 2023-01-03T08:15:41Z: https://ci.nodejs.org/job/node-test-pull-request/48821/ - Querying data for job/node-test-pull-request/48821/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/3828266911 |
Landed in e35e893 |
This change reduces the number of calls that were crossing the JS-C++ boundary to 1 and also removes the need for calling Array::New() multiple times internally and ArrayPrototypeConcat-ing the results later on, thus improving performance. Refs: nodejs#44445 (review) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: nodejs#46014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This change reduces the number of calls that were crossing the JS-C++ boundary to 1 and also removes the need for calling Array::New() multiple times internally and ArrayPrototypeConcat-ing the results later on, thus improving performance. Refs: #44445 (review) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #46014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This change reduces the number of calls that were crossing the JS-C++ boundary to 1 and also removes the need for calling Array::New() multiple times internally and ArrayPrototypeConcat-ing the results later on, thus improving performance. Refs: #44445 (review) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #46014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This change reduces the number of calls that were crossing the JS-C++ boundary to 1 and also removes the need for calling Array::New() multiple times internally and ArrayPrototypeConcat-ing the results later on, thus improving performance. Refs: #44445 (review) Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #46014 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This change reduces the number of calls that were crossing the JS-C++ boundary to 1 and also removes the need for calling
Array::New()
multiple times internally andArrayPrototypeConcat
-ing the results later on, thus improving performance by 75%!Refs: #44445 (review)
Signed-off-by: Darshan Sen raisinten@gmail.com
process.getActiveResourcesInfo()
benchmark result: 75% improvementtimers
benchmark result: no noticeable deterioration