Min time becomes measurement option - #424
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.
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.
|
This work is superseded by #439. The important drawback here is that while making This pull request should be closed without merging once #439 lands. |
--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