Uh oh!
There was an error while loading. Please reload this page.
feat(bench): #205 AC#5 enforce <= 50 ms warm threshold in bench-fastled-examples - #228
Merged
Merged
Conversation
…ed-examples AC#5 requires the warm-cache library-selection step to stay within +50 ms of current fbuild on the FastLED examples matrix. The bench-fastled-examples binary now accepts --max-warm-ms <f64>; when set, it exits 1 (after printing the full table) if any example's warm timing exceeds the threshold, listing every breach in both the error message and the JSON report. The bench-205 fastled-examples workflow job is no longer workflow_dispatch- only — it runs on every PR touching the resolver crates, the bench dir, or the workflow itself, and passes --max-warm-ms 50. The 50 ms cap gives ~25x headroom over current ~1-2 ms warm timings on developer hardware (and ~10 ms on CI), absorbing runner noise without false positives. Refs #205 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements AC#5 warm-cache threshold enforcement for the fastled-examples benchmark. It adds a ChangesAC#5 Warm-Cache Threshold Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/fastled-examples/src/main.rs`:
- Around line 288-300: parse_path_flag currently treats a bare "--json" (or
other flag) as absent; change it to fail fast when the flag is present but has
no value: update parse_path_flag to return a Result<Option<PathBuf>, String> (or
another error type used in the crate), detect the case where arg == flag and
return Err("flag '<flag>' requires a value") instead of consuming the next arg
as optional, and keep the existing strip_prefix behavior for "--flag=value".
Finally, update call sites that use parse_path_flag to propagate or handle the
error (e.g., print a clear message and exit) so CLI mistakes produce explicit
failures rather than silently hiding the flag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28fbedc3-4b47-47d8-8adf-9efb2945a152
📒 Files selected for processing (3)
.github/workflows/bench-205.ymlbench/fastled-examples/README.mdbench/fastled-examples/src/main.rs
Comment on lines
+288
to
300
| fn parse_path_flag(args: &[String], flag: &str) -> Option<PathBuf> { | ||
| let prefix = format!("{flag}="); | ||
| let mut iter = args.iter().skip(1); | ||
| while let Some(arg) = iter.next() { | ||
| if arg == "--json" { | ||
| if arg == flag { | ||
| return iter.next().map(PathBuf::from); | ||
| } | ||
| if let Some(rest) = arg.strip_prefix("--json=") { | ||
| if let Some(rest) = arg.strip_prefix(prefix.as_str()) { | ||
| return Some(PathBuf::from(rest)); | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
Fail fast when --json is provided without a value.
parse_path_flag silently treats a trailing --json as “flag absent”, which hides CLI mistakes and can make expected report output disappear without an explicit error.
Proposed fix
-fn parse_path_flag(args: &[String], flag: &str) -> Option<PathBuf> {+fn parse_path_flag(+ args: &[String],+ flag: &str,+) -> Result<Option<PathBuf>, Box<dyn std::error::Error>> {
let prefix = format!("{flag}=");
let mut iter = args.iter().skip(1);
while let Some(arg) = iter.next() {
if arg == flag {
- return iter.next().map(PathBuf::from);+ let next = iter+ .next()+ .ok_or_else(|| format!("{flag} requires a value"))?;+ if next.is_empty() {+ return Err(format!("{flag} requires a non-empty path").into());+ }+ return Ok(Some(PathBuf::from(next)));
}
if let Some(rest) = arg.strip_prefix(prefix.as_str()) {
- return Some(PathBuf::from(rest));+ if rest.is_empty() {+ return Err(format!("{flag} requires a non-empty path").into());+ }+ return Ok(Some(PathBuf::from(rest)));
}
}
- None+ Ok(None)
}- let json_out = parse_path_flag(&args, "--json");+ let json_out = parse_path_flag(&args, "--json")?;Also applies to: 94-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/fastled-examples/src/main.rs` around lines 288 - 300, parse_path_flag
currently treats a bare "--json" (or other flag) as absent; change it to fail
fast when the flag is present but has no value: update parse_path_flag to return
a Result<Option<PathBuf>, String> (or another error type used in the crate),
detect the case where arg == flag and return Err("flag '<flag>' requires a
value") instead of consuming the next arg as optional, and keep the existing
strip_prefix behavior for "--flag=value". Finally, update call sites that use
parse_path_flag to propagate or handle the error (e.g., print a clear message
and exit) so CLI mistakes produce explicit failures rather than silently hiding
the flag.
Uh oh!
There was an error while loading. Please reload this page.
5 tasks
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Closes AC#5 of #205 by turning the previously workflow-dispatch-only
fastled-examplesbench job into a PR-blocking CI gate on the resolver's warm timings.AC#5 verbatim:Warm build of the FastLED examples matrix adds <= 50 ms over current fbuild on the library-selection step (measured in bench/fastled-examples).
Pragmatic enforcement: each warm
resolve_cachedcall must complete in <= 50 ms wall-clock. Current developer-hardware numbers are ~1-2 ms warm, so 50 ms gives ~25x headroom — more than enough to absorb CI-runner noise without false positives.What changed
bench/fastled-examples/src/main.rs— adds--max-warm-ms <f64>CLI flag. When set, after the table is printed, any example whosewarm_ms > thresholdis collected; if the list is non-empty the binary exits 1 with a message listing every breach. The threshold is echoed in the report header and recorded in the JSON report under newmax_warm_ms/breacheskeys. No new deps. Pre-existing report-only behaviour preserved when the flag is omitted..github/workflows/bench-205.yml— removes theworkflow_dispatch-only gate on thefastled-examplesjob; addsbench/fastled-examples/**to the path filter; the run step now passes--max-warm-ms 50; renamed job toBench (fastled-examples — AC#5 <= 50 ms warm).bench/fastled-examples/README.md— adds an "AC#5 enforcement" subsection covering the flag, the 50 ms threshold rationale, CI wiring, and a local-usage example. Updates the existing "CI" section to reflect the gate-on-every-PR behaviour.Files modified
.github/workflows/bench-205.ymlbench/fastled-examples/README.mdbench/fastled-examples/src/main.rsTest plan
uv run soldr cargo check -p fbuild-bench-fastled-examples— cleanuv run soldr cargo clippy --workspace --all-targets -- -D warnings— only pre-existing MSRV-mismatch warnings onmain, no new warningsuv run soldr cargo fmt --all -- --check— clean--max-warm-ms 50againstC:/Users/niteris/dev/fastled: exit 0, threshold printed in header (- Warm threshold: 50.00 ms), all six examples ~8-10 ms warm--max-warm-ms 0.001: exit 1, all six examples listed as breaches in the error message and JSONbreachesfieldfastled-examplesjob runs on this PR (touchesbench/fastled-examples/**+ workflow itself) and gates on the 50 ms thresholdRefs #205 (using
Refs, notCloses— AC#6 cold <= 200 ms on teensy41 is still open).Claude Code
Summary by CodeRabbit