node
node copied to clipboard
nodejs
benchmark: adds groups to better separate benchmarks
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 https://github.com/nodejs/node/pull/39285
Fixes: https://github.com/nodejs/node/issues/26425
@H4ad thanks for taking a look! I have just pushed to update the documentation!
@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?
@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.4498875705796unfortunately 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 😄
About this https://github.com/nodejs/node/pull/54393#issuecomment-2295139627, 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.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
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
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@H4ad great! thanks for your review! I just pushed again to fix the linting issue in the docs
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/.ncuhttps://github.com/nodejs/node/actions/runs/10583649478
Just a last /cc @nodejs/performance in case anyone wants to take a look before merging.