tools: add `ArrayPrototypeConcat` to the list of primordials to avoid by aduh95 · Pull Request #44445 · nodejs/node · GitHub

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

/cc @nodejs/tsc for reviews

lgtm

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

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()......

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

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

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

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

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>

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.

Feel free to remove it when revisited.

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 outstanding review comments, 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 labels Jan 5, 2023

aduh95 removed dont-land-on-v14.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

luketaher mentioned this pull request Sep 4, 2024

performance.getEntries and performance.getEntriesByName cause call stack size to be exceeded #54768

Closed

simon-id mentioned this pull request Oct 9, 2024

diagnostics_channel: fix unsubscribe during publish #55116

Merged

tools: add `ArrayPrototypeConcat` to the list of primordials to avoid by aduh95 · Pull Request #44445 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

tools: add ArrayPrototypeConcat to the list of primordials to avoid#44445nodejs-github-bot merged 2 commits intonodejs:mainnodejs/node:mainfrom aduh95:lint-isConcatSpreadableaduh95/node:lint-isConcatSpreadableCopy head branch name to clipboard

Conversation

aduh95 commented Aug 29, 2022

Uh oh!

aduh95 commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Dec 15, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 15, 2022

Uh oh!

joyeecheung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 16, 2022

Uh oh!

nodejs-github-bot commented Dec 16, 2022

Uh oh!

nodejs-github-bot commented Dec 17, 2022

Uh oh!

nodejs-github-bot commented Dec 17, 2022

Uh oh!

nodejs-github-bot commented Dec 17, 2022

Uh oh!

RafaelGSS commented Jan 5, 2023

Uh oh!

RafaelGSS commented Jan 5, 2023

Uh oh!

aduh95 commented Jan 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tools: add ArrayPrototypeConcat to the list of primordials to avoid#44445
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
aduh95:lint-isConcatSpreadable

aduh95 commented Sep 6, 2022 •
edited
Loading

joyeecheung left a comment •
edited
Loading