benchmark: adds groups to better separate benchmarks

[puskin94]

Wandering around the codebase I found the TODO in benchmark/http/headers.js and, following the link, I discovered there was a stale PR which needed some love in order to make the TODO disappear.
Took inspiration from #39285

Fixes: #26425

[nodejs-github-bot]

Review requested:

• @nodejs/tsc

[puskin94]

Asking the future reviewer to re-trigger the pipeline due to flaky tests 😄

[H4ad]

Hey, thanks for the PR, it looks great!

Can you please add a section to explain how to run the benchmarks filtering by groups? After filtering benchmarks section, you can say there are benchmarks that have groups and you can run only one or more of these groups, and put some examples of how to filter via env variable.

Eg: NODE_RUN_BENCHMARK_GROUPS=fewHeaders,manyHeaders node http/headers.js

Maybe in the future we should create a flag such as --filter.

/cc @nodejs/performance @BridgeAR

[puskin94]

@H4ad thanks for taking a look! I have just pushed to update the documentation!

[puskin94]

@H4ad I had to update test/common/benchmark.js's regex to allow multiple lines from the output because now we have cases like this:

http/end-vs-write-end.js
http/end-vs-write-end.js duration=0.05 method="write" c=1 len=1 type="asc" benchmarker="test-double-http": 307

http/headers.js
http/headers.js group="fewHeaders" duration=5 len=1 n=10 benchmarker="test-double-http": 120,485
http/headers.js group="mediumHeaders" duration=5 len=1 n=10 benchmarker="test-double-http": 117,190
http/headers.js group="manyHeaders" duration=5 len=1 n=10 benchmarker="test-double-http": 117,381

http/http_server_for_chunky_client.js
http/_chunky_http_client.js type="send" n=1 len=1: 465.4498875705796

unfortunately this kinda defeats the purpose of the whole file: making sure that only one log is outputted from each benchmark file. Any idea? Does this file still make sense to be executed according to you?

[puskin94]

@H4ad I had to update test/common/benchmark.js's regex to allow multiple lines from the output because now we have cases like this:

http/end-vs-write-end.js
http/end-vs-write-end.js duration=0.05 method="write" c=1 len=1 type="asc" benchmarker="test-double-http": 307

http/headers.js
http/headers.js group="fewHeaders" duration=5 len=1 n=10 benchmarker="test-double-http": 120,485
http/headers.js group="mediumHeaders" duration=5 len=1 n=10 benchmarker="test-double-http": 117,190
http/headers.js group="manyHeaders" duration=5 len=1 n=10 benchmarker="test-double-http": 117,381

http/http_server_for_chunky_client.js
http/_chunky_http_client.js type="send" n=1 len=1: 465.4498875705796

unfortunately this kinda defeats the purpose of the whole file: making sure that only one log is outputted from each benchmark file. Any idea? Does this file still make sense to be executed according to you?

Ignore this, I converted the script to keep groups in consideration 😄

[H4ad]

About this #54393 (comment), I will need to double-check if the scripts related to the benchmark output file will not be broken with this change, so I will try to take a look as soon as possible.

[H4ad]

The compare.js worked without issues, so LGTM, thanks for the contribution!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.32%. Comparing base (c1ec099) to head (8a06104).
Report is 182 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54393      +/-   ##
==========================================
+ Coverage   87.08%   87.32%   +0.23%     
==========================================
  Files         648      648              
  Lines      182217   182359     +142     
  Branches    34956    34982      +26     
==========================================
+ Hits       158684   159241     +557     
+ Misses      16819    16382     -437     
- Partials     6714     6736      +22     

see 70 files with indirect coverage changes

[puskin94]

@H4ad great! thanks for your review! I just pushed again to fix the linting issue in the docs

[puskin94]

is anything missing from this PR? could it get merged?

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/54393
✔  Done loading data for nodejs/node/pull/54393
----------------------------------- PR info ------------------------------------
Title      benchmark: adds groups to better separate benchmarks (#54393)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     puskin94:benchmarks-by-groups -> nodejs:main
Labels     http, benchmark, commit-queue-squash
Commits    6
 - benchmark: adds groups to better separate benchmarks
 - doc: adjusted benchmarks docs to describe how to use the byGroups flag
 - benchmark: updated the benchmark.js file to keep in consideration groups
 - benchmark: updated the benchmark.js script to be more strict
 - doc: update benchmark description
 - doc: linting in benchmark docs and in comments in benchmark.js
Committers 2
 - Giovanni <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54393
Fixes: https://github.com/nodejs/node/issues/26425
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54393
Fixes: https://github.com/nodejs/node/issues/26425
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 15 Aug 2024 13:03:49 GMT
   ✔  Approvals: 1
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/54393#pullrequestreview-2264147757
   ✔  Last GitHub CI successful
   ✘  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[H4ad]

Just a last /cc @nodejs/performance in case anyone wants to take a look before merging.

[nodejs-github-bot]

Landed in 03fe00e