benchmark: Add NodeError and Error benchmark by RafaelGSS · Pull Request #43077 · nodejs/node

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.js
error/error.js disableEntropyCache=0 n=10000000: 506,239.3888750024
error/error.js disableEntropyCache=1 n=10000000: 490,143.20360859093
rafaelgss@rafaelgss-desktop:~/repos/os/node$ ./node benchmark/error/node-error.js
error/node-error.js disableEntropyCache=0 n=10000000: 159,137.83392330573
error/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'); 
        
           const runBenchmark = 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');

const runBenchmark = 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 <matteo.collina@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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

Landed in 0903515

PR-URL: #43077 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#43077 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #43077 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs/node#43077 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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 resolved Hide resolved

RafaelGSS force-pushed the benchmark/node-error branch 2 times, most recently from 7a0b11e to f5ece4a Compare 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 benchmark May 24, 2022

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

github-actions bot 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 64fc8c1 Compare 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 df87435 Compare May 25, 2022 23:56

github-actions bot 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-actions bot 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 da88923 Compare 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-actions bot 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 closed this May 30, 2022

github-actions bot 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-actions bot mentioned this pull request Jun 1, 2022

CI Reliability 2022-06-01 nodejs/reliability#295

Open

17 tasks

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

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