Min time becomes measurement option - #424
Draft
oleksandr-pavlyk wants to merge 13 commits into
Draft
Conversation
min-time used to be stdrel-stopping-criterion parameter, but was being used by hot measuring loop (see NVIDIA#408). This PR add m_min_time data member to benchmark_base and state classes as measurement option, same as `--min-samples`. Setter/getter methods are added. Measuring loops have access to this parameter, but delegate what to use with measurement options (--min-time and --min-samples) to individual stopping criteria. Stopping criteria acquired `bool is_elibigible_to_stop(stopping_context)` method with stopping_context populated with number-of-samples collected, min-samples, total-sample-time, min-time. The method allows stopping criteria to customize treatment of measurement options: * stdrel: honors both --min-time and --min-samples * entropy: honors --min-samples, ignores --min-time * target-samples: ignores both Closes NVIDIA#408. CLI option `--min-time` is now always recognized, irrespective of which stopping-criterion is current during option parsing.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (34)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change models ChangesMinimum measurement time
Assessment against linked issues
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Add a separate hot batch target time with a 100 us floor so --min-time 0, or very small positive min-time values, do not produce degenerate one-launch hot batches. Keep the user-provided min-time unchanged for stopping and timeout diagnostics; only the internal batch-size estimate uses the clamped target.
oleksandr-pavlyk
force-pushed
the
min-time-is-measurement-option
branch
from
July 22, 2026 17:25
a753dd8 to
0e4748d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
oleksandr-pavlyk
marked this pull request as ready for review
July 22, 2026 19:30
oleksandr-pavlyk
marked this pull request as draft
July 25, 2026 17:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--min-timeis now a part ofnvbench::benchmark_base,nvbench::state.min-time used to be stdrel-stopping-criterion parameter, but
was being used by hot measuring loop (see #408).
This PR add
m_min_timedata member tobenchmark_baseandstateclasses as measurement option, same as
--min-samples. Setter/gettermethods are added.
Option parser is modified to recognize
--min-time <duration>as option associatedwith benchmark and calls
.set_min_time(duration)setter.Measuring loops have access to min-time parameter, but delegate
what to do with measurement options (
--min-timeand--min-samples)to individual stopping criteria via new stopping criteria method
bool is_elibigible_to_stop(stopping_context context).The
stopping_contextargument contains the number-of-samples collected,min-samples, total-sample-time, min-time. The method allows stopping
criteria to customize treatment of measurement options:
--min-timeand--min-samples(same as before)--min-samples, ignores--min-time(same as before)Closes #408.
CLI option
--min-timeis now always recognized, irrespective of whichstopping-criterion is current during option parsing.
Python API modified to match C++.
docs/cli_help.mdmodified:--min-timemoved to measurement options section