Skip to content

benchmark: Add NodeError and Error benchmark#43077

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

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

Conversation

@RafaelGSS
Copy link
Copy Markdown
Member

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

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

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

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
Copy link
Copy Markdown
Collaborator

@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

@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

@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented May 25, 2022

Can you add a test, similarly as

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

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

Can you add a test, similarly as

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

Just did, I knew I was forgetting something.

@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