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

benchmark: Add NodeError and Error benchmark#43077

Closed
RafaelGSS wants to merge 1 commit intonodejs:masterfrom
RafaelGSS:benchmark/node-error
Closed

benchmark: Add NodeError and Error benchmark#43077
RafaelGSS wants to merge 1 commit intonodejs:masterfrom
RafaelGSS:benchmark/node-error

Conversation

@RafaelGSS
Copy link
Copy Markdown
Member

@RafaelGSS RafaelGSS commented May 12, 2022

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

@nodejs-github-bot 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 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
@RafaelGSS
Copy link
Copy Markdown
Member Author

RafaelGSS commented May 12, 2022

mscdex
mscdex reviewed May 12, 2022
View reviewed changes
Comment thread lib/util.js Outdated
@mscdex
Copy link
Copy Markdown
Contributor

mscdex commented May 12, 2022

What is the purpose of disableEntropyCache?

@RafaelGSS RafaelGSS force-pushed the benchmark/node-error branch 2 times, most recently from 7a0b11e to f5ece4a Compare May 24, 2022 14:47
mcollina
mcollina approved these changes May 24, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@RafaelGSS
Copy link
Copy Markdown
Member Author

RafaelGSS commented May 24, 2022

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

@RafaelGSS RafaelGSS marked this pull request as ready for review May 24, 2022 15:04
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2022
@RafaelGSS RafaelGSS changed the title bench: node-error benchmark benchmark: Add NodeError and Error benchmark May 24, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2022
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 24, 2022

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 24, 2022

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

@github-actions github-actions Bot mentioned this pull request May 25, 2022
Open
19 tasks
@RafaelGSS 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
@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented May 25, 2022

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

@RafaelGSS RafaelGSS force-pushed the benchmark/node-error branch from f5ece4a to 64fc8c1 Compare May 25, 2022 20:28
@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented May 25, 2022

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

@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented May 25, 2022

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
juanarbol approved these changes May 25, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RafaelGSS RafaelGSS force-pushed the benchmark/node-error branch from 64fc8c1 to df87435 Compare May 25, 2022 23:56
@RafaelGSS
Copy link
Copy Markdown
Member Author

RafaelGSS commented May 25, 2022

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.

@github-actions github-actions Bot mentioned this pull request May 26, 2022
Open
19 tasks
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2022
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 26, 2022

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 26, 2022

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 27, 2022

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

@RafaelGSS RafaelGSS force-pushed the benchmark/node-error branch from df87435 to da88923 Compare May 27, 2022 11:47
@RafaelGSS
Copy link
Copy Markdown
Member Author

RafaelGSS commented May 27, 2022

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

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 27, 2022

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

This was referenced May 28, 2022
Open
Open
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 29, 2022

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

@github-actions github-actions Bot mentioned this pull request May 30, 2022
Open
18 tasks
@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label May 30, 2022
@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented May 30, 2022

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

@RafaelGSS 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
@RafaelGSS
benchmark: add node-error benchmark
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>
@RafaelGSS
Copy link
Copy Markdown
Member Author

RafaelGSS commented May 30, 2022

@RafaelGSS RafaelGSS closed this May 30, 2022
bengl pushed a commit that referenced this pull request May 30, 2022
@RafaelGSS @bengl
benchmark: add node-error benchmark
aefc9dd 
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>
@github-actions github-actions Bot mentioned this pull request May 31, 2022
Open
16 tasks
@bengl bengl mentioned this pull request May 31, 2022
Merged
@github-actions github-actions Bot mentioned this pull request Jun 1, 2022
Open
17 tasks
F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 1, 2022
@RafaelGSS @F3n67u
benchmark: add node-error benchmark
da1e302 
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>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@RafaelGSS @danielleadams
benchmark: add node-error benchmark
d5e745d 
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>
targos pushed a commit that referenced this pull request Jul 12, 2022
@RafaelGSS @targos
benchmark: add node-error benchmark
5f85999 
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>
targos pushed a commit that referenced this pull request Jul 31, 2022
@RafaelGSS @targos
benchmark: add node-error benchmark
986fb3e 
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>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@RafaelGSS @guangwong
benchmark: add node-error benchmark
6b08829 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@aduh95 aduh95 aduh95 approved these changes

@juanarbol juanarbol juanarbol approved these changes

+1 more reviewer

@mscdex mscdex mscdex left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@RafaelGSS @mscdex @nodejs-github-bot @aduh95 @mcollina @juanarbol