benchmark: Add NodeError and Error benchmark#43077

RafaelGSS · 2022-05-12T19:26:45Z

While investigating fetch performance (nodejs/undici#1203 (comment)) I've found that NodeError is a bottleneck in WebStreams.

I created this Pull Request to share the insights and help as I can to improve the NodeError performance. We can use this PR thread to discuss approaches to solve it.

For reference, this is the benchmark result I'm getting:

*-cpu description: CPU product: AMD Ryzen 5 3500X 6-Core Processor vendor: Advanced Micro Devices [AMD] physical id: 2f bus info: cpu@0 version: AMD Ryzen 5 3500X 6-Core Processor serial: Unknown slot: AM4 size: 4030MHz capacity: 4100MHz width: 64 bits clock: 100MHz

rafaelgss@rafaelgss-desktop:~/repos/os/node$ ./node benchmark/error/error.jserror/error.js disableEntropyCache=0 n=10000000: 506,239.3888750024error/error.js disableEntropyCache=1 n=10000000: 490,143.20360859093rafaelgss@rafaelgss-desktop:~/repos/os/node$ ./node benchmark/error/node-error.jserror/node-error.js disableEntropyCache=0 n=10000000: 159,137.83392330573error/node-error.js disableEntropyCache=1 n=10000000: 159,349.56687673234

cc: @jasnell @mcollina

RafaelGSS · 2022-05-12T19:28:07Z

Furthermore, I can go deeper into the analysis and share my thoughts here.

lib/util.js

mscdex · 2022-05-12T21:16:07Z

What is the purpose of disableEntropyCache?

mcollina

lgtm

RafaelGSS · 2022-05-24T15:04:34Z

Well, lately I have had no time to investigate/improve how the NodeError is managed. I think it's ready to land with just the benchmark. I plan to back to it whenever I have free time.

A few insights to solve it can be found in: nodejs/undici#1203 (comment).

nodejs-github-bot · 2022-05-24T15:19:29Z

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

nodejs-github-bot · 2022-05-24T17:53:03Z

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

aduh95 · 2022-05-25T14:27:57Z

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

aduh95 · 2022-05-25T23:03:58Z

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

aduh95 · 2022-05-25T23:06:01Z

Can you add a test, similarly as

node/test/benchmark/test-benchmark-misc.js

Lines 1 to 7 in 9e40df7

'use strict';
require('../common');
construnBenchmark=require('../common/benchmark');
runBenchmark('misc',{NODEJS_BENCHMARK_ZERO_ALLOWED: 1});

juanarbol

LGTM

RafaelGSS · 2022-05-25T23:57:19Z

Can you add a test, similarly as
node/test/benchmark/test-benchmark-misc.js
Lines 1 to 7 in 9e40df7
'use strict';
require('../common');
construnBenchmark=require('../common/benchmark');
runBenchmark('misc',{NODEJS_BENCHMARK_ZERO_ALLOWED: 1});

Just did, I knew I was forgetting something.

nodejs-github-bot · 2022-05-26T16:43:54Z

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

nodejs-github-bot · 2022-05-26T22:16:06Z

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

nodejs-github-bot · 2022-05-27T00:15:59Z

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

RafaelGSS · 2022-05-27T11:48:43Z

Just rebased on master to check if the flaky tests are gone

nodejs-github-bot · 2022-05-27T11:52:59Z

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

nodejs-github-bot · 2022-05-29T18:37:24Z

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

nodejs-github-bot · 2022-05-30T12:24:18Z

Commit Queue failed

- Loading data for nodejs/node/pull/43077 ✔ Done loading data for nodejs/node/pull/43077 ----------------------------------- PR info ------------------------------------ Title benchmark: Add NodeError and Error benchmark (#43077) Author Rafael Gonzaga (@RafaelGSS) Branch RafaelGSS:benchmark/node-error -> nodejs:master Labels discuss, author ready, needs-ci Commits 1 - benchmark: add node-error benchmark Committers 1 - RafaelGSS PR-URL: https://github.com/nodejs/node/pull/43077 Reviewed-By: Matteo Collina Reviewed-By: Juan José Arboleda Reviewed-By: Antoine du Hamel ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/43077 Reviewed-By: Matteo Collina Reviewed-By: Juan José Arboleda Reviewed-By: Antoine du Hamel -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - benchmark: add node-error benchmark ℹ This PR was created on Thu, 12 May 2022 19:26:45 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/43077#pullrequestreview-983394804 ✔ - Juan José Arboleda (@juanarbol): https://github.com/nodejs/node/pull/43077#pullrequestreview-985584535 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/43077#pullrequestreview-985886311 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-05-29T18:37:24Z: https://ci.nodejs.org/job/node-test-pull-request/44228/ ℹ Last Benchmark CI on 2022-05-25T23:03:58Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1145/ - Querying data for job/node-test-pull-request/44228/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2409007414

PR-URL: #43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS · 2022-05-30T12:54:38Z

Landed in 0903515

PR-URL: #43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: nodejs#43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: #43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

PR-URL: nodejs/node#43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels May 12, 2022

RafaelGSS added discuss Issues opened for discussions and feedbacks. and removed util Issues and PRs related to the built-in util module. labels May 12, 2022

mscdex reviewed May 12, 2022
View reviewed changes

lib/util.js Outdated Show resolvedHide resolved

RafaelGSS force-pushed the benchmark/node-error branch 2 times, most recently from 7a0b11e to f5ece4aCompare May 24, 2022 14:47

mcollina approved these changes May 24, 2022
View reviewed changes

RafaelGSS marked this pull request as ready for review May 24, 2022 15:04

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2022

RafaelGSS changed the title ~~bench: node-error benchmark~~benchmark: Add NodeError and Error benchmarkMay 24, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2022

github-actionsbot mentioned this pull request May 25, 2022
CI Reliability 2022-05-25 nodejs/reliability#288
Open
19 tasks

RafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 25, 2022

RafaelGSS force-pushed the benchmark/node-error branch from f5ece4a to 64fc8c1Compare May 25, 2022 20:28

juanarbol approved these changes May 25, 2022
View reviewed changes

RafaelGSS force-pushed the benchmark/node-error branch from 64fc8c1 to df87435Compare May 25, 2022 23:56

github-actionsbot mentioned this pull request May 26, 2022
CI Reliability 2022-05-26 nodejs/reliability#289
Open
19 tasks

aduh95 approved these changes May 26, 2022
View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2022

benchmark: add node-error benchmark
da88923

RafaelGSS force-pushed the benchmark/node-error branch from df87435 to da88923Compare May 27, 2022 11:47

This was referenced May 28, 2022
CI Reliability 2022-05-28 nodejs/reliability#291
Open
CI Reliability 2022-05-29 nodejs/reliability#292
Open

github-actionsbot mentioned this pull request May 30, 2022
CI Reliability 2022-05-30 nodejs/reliability#293
Open
18 tasks

RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label May 30, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 30, 2022

RafaelGSS removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. discuss Issues opened for discussions and feedbacks. labels May 30, 2022

RafaelGSS added a commit that referenced this pull request May 30, 2022
benchmark: add node-error benchmark
0903515
PR-URL: #43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS closed this May 30, 2022

bengl pushed a commit that referenced this pull request May 30, 2022
benchmark: add node-error benchmark
aefc9dd
PR-URL: #43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

github-actionsbot mentioned this pull request May 31, 2022
CI Reliability 2022-05-31 nodejs/reliability#294
Open
16 tasks

bengl mentioned this pull request May 31, 2022
v18.3.0 proposal #43266
Merged

github-actionsbot mentioned this pull request Jun 1, 2022
CI Reliability 2022-06-01 nodejs/reliability#295
Open
17 tasks

danielleadams pushed a commit that referenced this pull request Jun 27, 2022
benchmark: add node-error benchmark
d5e745d
PR-URL: #43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

targos pushed a commit that referenced this pull request Jul 12, 2022
benchmark: add node-error benchmark
5f85999
PR-URL: #43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

targos pushed a commit that referenced this pull request Jul 31, 2022
benchmark: add node-error benchmark
986fb3e
PR-URL: #43077 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
Merged

Uh oh!

benchmark: Add NodeError and Error benchmark#43077

benchmark: Add NodeError and Error benchmark #43077

Uh oh!

Conversation

RafaelGSS commented May 12, 2022

Uh oh!

RafaelGSS commented May 12, 2022

Uh oh!

Uh oh!

mscdex commented May 12, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

RafaelGSS commented May 24, 2022

Uh oh!

nodejs-github-bot commented May 24, 2022

Uh oh!

nodejs-github-bot commented May 24, 2022

Uh oh!

aduh95 commented May 25, 2022

Uh oh!

aduh95 commented May 25, 2022

Uh oh!

aduh95 commented May 25, 2022

Uh oh!

juanarbol left a comment

Choose a reason for hiding this comment

Uh oh!

RafaelGSS commented May 25, 2022

Uh oh!

nodejs-github-bot commented May 26, 2022

Uh oh!

nodejs-github-bot commented May 26, 2022

Uh oh!

nodejs-github-bot commented May 27, 2022

Uh oh!

RafaelGSS commented May 27, 2022

Uh oh!

nodejs-github-bot commented May 27, 2022

Uh oh!

nodejs-github-bot commented May 29, 2022

Uh oh!

nodejs-github-bot commented May 30, 2022

Uh oh!

RafaelGSS commented May 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants