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.

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

tools: add ArrayPrototypeConcat to the list of primordials to avoid #44445

Merged
merged 2 commits into from
Dec 17, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 29, 2022

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. labels Aug 29, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Sep 6, 2022

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1266/

Results
                                                                                  confidence improvement accuracy (*)    (**)   (***)
module/module-loader-circular.js n=10000                                                          0.45 %       ±3.92%  ±5.22%  ±6.79%
module/module-loader-deep.js cache='false' files=1000 ext=''                               *     -4.75 %       ±4.42%  ±5.90%  ±7.72%
module/module-loader-deep.js cache='false' files=1000 ext='.js'                                   0.16 %       ±3.51%  ±4.67%  ±6.08%
module/module-loader-deep.js cache='true' files=1000 ext=''                                       1.24 %       ±3.10%  ±4.16%  ±5.48%
module/module-loader-deep.js cache='true' files=1000 ext='.js'                                    0.75 %       ±5.54%  ±7.45%  ±9.85%
module/module-loader.js cache='false' n=1000 files=500 dir='abs' name='/'                         0.31 %       ±3.83%  ±5.11%  ±6.67%
module/module-loader.js cache='false' n=1000 files=500 dir='abs' name=''                          0.20 %       ±7.00%  ±9.33% ±12.17%
module/module-loader.js cache='false' n=1000 files=500 dir='abs' name='/index.js'                 4.89 %       ±6.28%  ±8.42% ±11.09%
module/module-loader.js cache='false' n=1000 files=500 dir='rel' name='/'                        -0.62 %       ±2.76%  ±3.68%  ±4.79%
module/module-loader.js cache='false' n=1000 files=500 dir='rel' name=''                         -0.46 %       ±3.71%  ±4.94%  ±6.44%
module/module-loader.js cache='false' n=1000 files=500 dir='rel' name='/index.js'                 4.25 %       ±5.76%  ±7.71% ±10.14%
module/module-loader.js cache='false' n=1 files=500 dir='abs' name='/'                            0.24 %       ±6.88%  ±9.16% ±11.94%
module/module-loader.js cache='false' n=1 files=500 dir='abs' name=''                             0.05 %       ±6.70%  ±8.92% ±11.61%
module/module-loader.js cache='false' n=1 files=500 dir='abs' name='/index.js'                   -5.85 %       ±6.25%  ±8.31% ±10.82%
module/module-loader.js cache='false' n=1 files=500 dir='rel' name='/'                            2.81 %       ±6.29%  ±8.37% ±10.90%
module/module-loader.js cache='false' n=1 files=500 dir='rel' name=''                            -0.14 %       ±5.86%  ±7.80% ±10.15%
module/module-loader.js cache='false' n=1 files=500 dir='rel' name='/index.js'                   -5.70 %       ±6.15%  ±8.19% ±10.67%
module/module-loader.js cache='true' n=1000 files=500 dir='abs' name='/'                          1.67 %       ±2.44%  ±3.27%  ±4.30%
module/module-loader.js cache='true' n=1000 files=500 dir='abs' name=''                           2.31 %       ±6.46%  ±8.60% ±11.20%
module/module-loader.js cache='true' n=1000 files=500 dir='abs' name='/index.js'                  0.57 %       ±1.18%  ±1.59%  ±2.09%
module/module-loader.js cache='true' n=1000 files=500 dir='rel' name='/'                          0.44 %       ±2.16%  ±2.89%  ±3.78%
module/module-loader.js cache='true' n=1000 files=500 dir='rel' name=''                          -0.94 %       ±2.00%  ±2.68%  ±3.53%
module/module-loader.js cache='true' n=1000 files=500 dir='rel' name='/index.js'           *     -1.66 %       ±1.58%  ±2.12%  ±2.77%
module/module-loader.js cache='true' n=1 files=500 dir='abs' name='/'                            -0.82 %       ±4.57%  ±6.13%  ±8.10%
module/module-loader.js cache='true' n=1 files=500 dir='abs' name=''                             -0.40 %       ±4.99%  ±6.64%  ±8.64%
module/module-loader.js cache='true' n=1 files=500 dir='abs' name='/index.js'                    -1.51 %       ±4.19%  ±5.63%  ±7.43%
module/module-loader.js cache='true' n=1 files=500 dir='rel' name='/'                             1.88 %       ±2.96%  ±3.97%  ±5.23%
module/module-loader.js cache='true' n=1 files=500 dir='rel' name=''                              0.87 %       ±3.08%  ±4.10%  ±5.34%
module/module-loader.js cache='true' n=1 files=500 dir='rel' name='/index.js'                    -2.44 %       ±5.53%  ±7.35%  ±9.57%
module/module-require.js n=10000 type='dir'                                                       4.80 %       ±8.38% ±11.16% ±14.52%
module/module-require.js n=10000 type='.js'                                                *      6.75 %       ±6.61%  ±8.80% ±11.45%
module/module-require.js n=10000 type='.json'                                                    -6.52 %       ±7.31%  ±9.72% ±12.66%

@aduh95 aduh95 added the review wanted PRs that need reviews. label Dec 15, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Dec 15, 2022

/cc @nodejs/tsc for reviews

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

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 15, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2022
@nodejs-github-bot
Copy link
Collaborator

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.

This reminds me that we once had a JS function dedicated to pushing items into an array so that C++ can call that instead of Object::Set (which was slower), and I got rid of that by adding an Array::New() that takes an array of Local<Value> from C++. I wonder if it'd be faster now in cases like getActiveResourcesInfo() if we simply assemble the array from C++ using Array::New()......

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2022
@nodejs-github-bot nodejs-github-bot merged commit ca2ec90 into nodejs:main Dec 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in ca2ec90

RaisinTen added a commit to RaisinTen/node that referenced this pull request Dec 29, 2022
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>
targos pushed a commit that referenced this pull request Jan 1, 2023
PR-URL: #44445
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
nodejs-github-bot pushed a commit that referenced this pull request Jan 3, 2023
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>
@RafaelGSS
Copy link
Member

As mentioned in the v19.4.0 proposal (#46061 (comment)), this seems to be causing unexpected behavior in some modules. So, I'm including the flag dont-land-on-vXX until we validate whether this is a bug or not.

More details of the error can be found on the proposal CITGM.

@RafaelGSS RafaelGSS added dont-land-on-v14.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Jan 5, 2023
@RafaelGSS
Copy link
Member

Feel free to remove it when revisited.

@aduh95 aduh95 removed dont-land-on-v14.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels Jan 8, 2023
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 8, 2023

Removed the labels as #46108 fixed the unexpected behavior.

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
PR-URL: nodejs#44445
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
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>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
PR-URL: #44445
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
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>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
PR-URL: #44445
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
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>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants