Skip to content

benchmark: fix calibrate-n option handling#64146

Open
luanmuniz wants to merge 1 commit into
nodejs:mainfrom
luanmuniz:fix/calibrate-n-option-parsing
Open

benchmark: fix calibrate-n option handling#64146
luanmuniz wants to merge 1 commit into
nodejs:mainfrom
luanmuniz:fix/calibrate-n-option-parsing

Conversation

@luanmuniz

Copy link
Copy Markdown

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

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>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/performance

@nodejs-github-bot nodejs-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

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

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

Copy link
Copy Markdown
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants