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 <[email protected]>

PR-URL: #44445 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

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 <[email protected]> PR-URL: #46014 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

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 <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

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 <[email protected]> PR-URL: nodejs#46014 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

PR-URL: #44445 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

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 <[email protected]> PR-URL: #46014 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

PR-URL: #44445 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

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 <[email protected]> PR-URL: #46014 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

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 ca3ed36Compare 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 b67284eCompare 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-actionsbot 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-actionsbot 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:mainDec 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

Uh oh!

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

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

Uh oh!

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

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

aduh95 commented Sep 6, 2022•
edited
Loading

joyeecheung left a comment •
edited
Loading