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

aduh95 · 2022-08-29T22:33:34Z

aduh95 · 2022-09-06T08:30:38Z

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%

…o avoid

aduh95 · 2022-12-15T08:46:18Z

/cc @nodejs/tsc for reviews

mcollina

lgtm

nodejs-github-bot · 2022-12-15T12:09:39Z

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

joyeecheung

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 · 2022-12-16T00:09:03Z

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

nodejs-github-bot · 2022-12-16T16:18:18Z

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

nodejs-github-bot · 2022-12-17T10:34:33Z

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

nodejs-github-bot · 2022-12-17T14:16:45Z

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

nodejs-github-bot · 2022-12-17T19:15:26Z

Landed in ca2ec90

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: #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>

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 · 2023-01-05T14:54:11Z

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 · 2023-01-05T14:54:39Z

Feel free to remove it when revisited.

aduh95 · 2023-01-08T17:33:39Z

Removed the labels as #46108 fixed the unexpected behavior.

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>

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>

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>

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>

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>

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>

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

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

tools: add ArrayPrototypeConcat to the list of primordials to avoid

b67284e

aduh95 force-pushed the lint-isConcatSpreadable branch from a60b429 to b67284e Compare December 14, 2022 23:04

fixup! tools: add ArrayPrototypeConcat to the list of primordials t…

d3761c2

…o avoid

aduh95 added the review wanted PRs that need reviews. label Dec 15, 2022

mcollina approved these changes Dec 15, 2022

View reviewed changes

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 bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 15, 2022

joyeecheung approved these changes Dec 15, 2022

View reviewed changes

RafaelGSS approved these changes Dec 16, 2022

View reviewed changes

tniessen approved these changes Dec 16, 2022

View reviewed changes

github-actions bot mentioned this pull request Dec 17, 2022

CI Reliability 2022-12-17 nodejs/reliability#460

Open

13 tasks

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2022

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 merged commit ca2ec90 into nodejs:main Dec 17, 2022

aduh95 deleted the lint-isConcatSpreadable branch December 17, 2022 19:22

This was referenced Dec 18, 2022

CI Reliability 2022-12-18 nodejs/reliability#461

Open

CI Reliability 2022-12-19 nodejs/reliability#462

Open

CI Reliability 2022-12-20 nodejs/reliability#463

Open

RaisinTen mentioned this pull request Dec 29, 2022

src: speed up process.getActiveResourcesInfo() #46014

Merged

RafaelGSS mentioned this pull request Jan 2, 2023

v19.4.0 proposal #46061

Merged

RafaelGSS added the dont-land-on-v19.x label Jan 5, 2023

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

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

RafaelGSS mentioned this pull request Jan 20, 2023

v19.5.0 proposal #46286

Merged

juanarbol mentioned this pull request Jan 28, 2023

V18.14.0 proposal #46396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aduh95 commented Aug 29, 2022

aduh95 commented Sep 6, 2022 •

edited

aduh95 commented Dec 15, 2022

mcollina left a comment

nodejs-github-bot commented Dec 15, 2022

joyeecheung left a comment •

edited

nodejs-github-bot commented Dec 16, 2022

nodejs-github-bot commented Dec 16, 2022

nodejs-github-bot commented Dec 17, 2022

nodejs-github-bot commented Dec 17, 2022

nodejs-github-bot commented Dec 17, 2022

RafaelGSS commented Jan 5, 2023

RafaelGSS commented Jan 5, 2023

aduh95 commented Jan 8, 2023

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

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

Conversation

aduh95 commented Aug 29, 2022

aduh95 commented Sep 6, 2022 • edited

aduh95 commented Dec 15, 2022

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 15, 2022

joyeecheung left a comment • edited

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 16, 2022

nodejs-github-bot commented Dec 16, 2022

nodejs-github-bot commented Dec 17, 2022

nodejs-github-bot commented Dec 17, 2022

nodejs-github-bot commented Dec 17, 2022

RafaelGSS commented Jan 5, 2023

RafaelGSS commented Jan 5, 2023

aduh95 commented Jan 8, 2023

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

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

aduh95 commented Sep 6, 2022 •

edited

joyeecheung left a comment •

edited