Skip to content

benchmark: fix calibrate-n option handling - #64146

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
luanmuniz:fix/calibrate-n-option-parsing
Jul 29, 2026
Merged

benchmark: fix calibrate-n option handling#64146
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
luanmuniz:fix/calibrate-n-option-parsing

Conversation

@luanmuniz

@luanmunizluanmuniz commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

This fixes a few issues in benchmark/calibrate-n.js found while using the tool to calibrate new benchmark values.

Before this change, --cv-threshold and --max-increases were parsed from the wrong substring index. The parsed value still included the = separator, so values such as --cv-threshold=0.035 and --max-increases=1 were read as =0.035 and =1. This made --cv-threshold become NaN and caused --max-increases to fall back to the default value.

This also updates the stability checks and output so the configured threshold is used consistently. Previously, parts of the tool still compared the overall CV against the default CV_THRESHOLD and printed hardcoded 5% / 10% thresholds even when custom values were provided.

Changes included:

  • fix parsing for --cv-threshold
  • fix parsing for --max-increases
  • use the configured cvThreshold when checking overall stability
  • validate invalid --runs and --cv-threshold values consistently with the other numeric options
  • add named constants for the default starting n value and the per-config CV threshold
  • correct the --start-n help text to match the actual already-in-place default value
  • update user-facing output so reported thresholds match the values used by the tool

@nodejs-github-bot

nodejs-github-bot commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-botnodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. performance Issues and PRs related to the performance of Node.js. labels Jun 26, 2026
@luanmunizluanmuniz mentioned this pull request Jun 26, 2026
Open
RafaelGSS
RafaelGSS approved these changes Jun 26, 2026
View reviewed changes

@RafaelGSSRafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@RafaelGSSRafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 26, 2026
@luanmuniz
benchmark: fix calibrate-n option handling
edb29d4
Fix broken parsing for --cv-threshold and --max-increases so the
value after the equals sign is used.
Use the configured CV threshold when checking overall stability and
report the configured thresholds in calibrate-n output. Also handle
invalid numeric values for --runs and --cv-threshold consistently
with the other numeric options.
Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
@luanmuniz

luanmuniz commented Jun 26, 2026

Copy link
Copy Markdown
ContributorAuthor

@RafaelGSS Thanks for the review! I can see the CI is failing due to an issue with the commit message.

I noticed the same problem on my other open PR's, and I already fixed them there by amend+force-push-with-lease.

I reread the guidelines to make sure, and they say that for this type of issue, someone would help get things sorted here. I will await your instructions on the correct flow to fix the CI here.

@RafaelGSS

RafaelGSS commented Jun 29, 2026

Copy link
Copy Markdown
Member

@luanmuniz the commit message line should be <= 72 columns. Please commit amend + force push

@luanmuniz
luanmunizforce-pushed the fix/calibrate-n-option-parsing branch from ae12dfc to edb29d4CompareJune 29, 2026 13:06
@luanmuniz

luanmuniz commented Jun 29, 2026

Copy link
Copy Markdown
ContributorAuthor

@RafaelGSS Done

@luanmuniz

luanmuniz commented Jul 10, 2026

Copy link
Copy Markdown
ContributorAuthor

@RafaelGSS Sorry to keep pinging, im waiting the existing PR's to be closed to continue working on the other benchmarks, i don't want to have more PR's open at the same time. Let me know if there is anything i can do to help you move this and the other tickets i have open. Thank you!

@luanmuniz

luanmuniz commented Jul 17, 2026

Copy link
Copy Markdown
ContributorAuthor

@RafaelGSS @nodejs/benchmarking Friendly follow-up on this one.

The current commit fix the commit-message wrapping as requested. Since the commit queue requires approval and successful CI after the latest change, could someone please reapprove the current HEAD, confirm or rerun CI?

Thank you

@trivikrtrivikr added request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jul 29, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 29, 2026
@nodejs-github-bot
nodejs-github-bot merged commit ba131bf into nodejs:mainJul 29, 2026
37 checks passed
@nodejs-github-bot

nodejs-github-bot commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Landed in ba131bf

aduh95 pushed a commit that referenced this pull request Aug 3, 2026
@luanmuniz@aduh95
benchmark: fix calibrate-n option handling
310342f
Fix broken parsing for --cv-threshold and --max-increases so the
value after the equals sign is used.
Use the configured CV threshold when checking overall stability and
report the configured thresholds in calibrate-n output. Also handle
invalid numeric values for --runs and --cv-threshold consistently
with the other numeric options.
Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
PR-URL: #64146
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
aduh95 pushed a commit that referenced this pull request Aug 4, 2026
@luanmuniz@aduh95
benchmark: fix calibrate-n option handling
6eb33c2
Fix broken parsing for --cv-threshold and --max-increases so the
value after the equals sign is used.
Use the configured CV threshold when checking overall stability and
report the configured thresholds in calibrate-n output. Also handle
invalid numeric values for --runs and --cv-threshold consistently
with the other numeric options.
Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
PR-URL: #64146
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@github-actionsgithub-actionsBot mentioned this pull request Aug 4, 2026
Merged
aduh95 pushed a commit that referenced this pull request Aug 5, 2026
@luanmuniz@aduh95
benchmark: fix calibrate-n option handling
d7266cd
Fix broken parsing for --cv-threshold and --max-increases so the
value after the equals sign is used.
Use the configured CV threshold when checking overall stability and
report the configured thresholds in calibrate-n output. Also handle
invalid numeric values for --runs and --cv-threshold consistently
with the other numeric options.
Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
PR-URL: #64146
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
aduh95 pushed a commit that referenced this pull request Aug 6, 2026
@luanmuniz@aduh95
benchmark: fix calibrate-n option handling
ba2612c
Fix broken parsing for --cv-threshold and --max-increases so the
value after the equals sign is used.
Use the configured CV threshold when checking overall stability and
report the configured thresholds in calibrate-n output. Also handle
invalid numeric values for --runs and --cv-threshold consistently
with the other numeric options.
Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
PR-URL: #64146
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RafaelGSSRafaelGSSRafaelGSS approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.benchmarkIssues and PRs related to the benchmark subsystem.performanceIssues and PRs related to the performance of Node.js.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@luanmuniz@nodejs-github-bot@RafaelGSS@trivikr