Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .github/workflows/bench-205.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ on:
- 'crates/fbuild-header-scan/**'
- 'crates/fbuild-library-select/**'
- 'crates/fbuild-test-support/src/mini_framework.rs'
- 'bench/fastled-examples/**'
- '.github/workflows/bench-205.yml'

env:
Expand Down Expand Up @@ -57,8 +58,7 @@ jobs:
retention-days: 14

fastled-examples:
name: Bench (fastled-examples — workflow_dispatch only)
if: github.event_name == 'workflow_dispatch'
name: Bench (fastled-examples — AC#5 ≤ 50 ms warm)
runs-on: ubuntu-latest
timeout-minutes: 30
steps:
Expand All @@ -81,12 +81,13 @@ jobs:
# bench/fastled-examples/README.md when retaking baseline.
ref: "3.10.3"

- name: Run bench-fastled-examples
- name: Run bench-fastled-examples (AC#5 enforcement, threshold 50 ms)
env:
FASTLED_DIR: ${{ github.workspace }}/external/fastled
run: |
mkdir -p bench/fastled-examples
soldr cargo run --release -p fbuild-bench-fastled-examples -- \
--max-warm-ms 50 \
--json bench/fastled-examples/report.json | tee bench/fastled-examples/report.md

- name: Upload report
Expand Down
43 changes: 33 additions & 10 deletions bench/fastled-examples/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,36 @@ FASTLED_DIR=/path/to/fastled \
FASTLED_DIR=/path/to/fastled \
uv run soldr cargo run --release -p fbuild-bench-fastled-examples \
-- --json bench/fastled-examples/report.json

# Enforce AC#5 (≤ 50 ms warm per example). Exits 1 on breach.
FASTLED_DIR=/path/to/fastled \
uv run soldr cargo run --release -p fbuild-bench-fastled-examples \
-- --max-warm-ms 50
```

If any example fails to measure (missing sketch, KvStore error, warm
miss) the binary exits non-zero rather than skipping the row. CI must
treat a partial matrix as a failure, not a pass.

## AC#5 enforcement

`--max-warm-ms <f64>` is the CI gate for AC#5. When provided, the
binary prints the full results table, then exits 1 if any example's
warm timing exceeds the threshold; the error message lists every
breach. The threshold is also echoed in the report header (`- Warm
threshold: 50.00 ms`) and recorded under the top-level `max_warm_ms`
key in the JSON report, with the breach list under `breaches`.

The `fastled-examples` job in `.github/workflows/bench-205.yml` passes
`--max-warm-ms 50`. The 50 ms value gives roughly 25× headroom over
the current ~1-2 ms warm timings on developer hardware (and ~10 ms on
CI runners), comfortably absorbing runner noise without false
positives. The job runs on every PR whose changes touch
`crates/fbuild-library-select/**`, `crates/fbuild-header-scan/**`,
`bench/fastled-examples/**`, the shared `MiniFramework` fixture, or
the workflow itself — gating those PRs on the resolver staying within
the AC#5 envelope.

## Sample numbers

Captured 2026-05-10 on Windows / Ryzen workstation, FastLED `main`,
Expand Down Expand Up @@ -77,16 +101,15 @@ The current set spans:

## CI

The `fastled-examples` job in `.github/workflows/bench-205.yml` is
`workflow_dispatch`-only because it requires a FastLED checkout. CI
checks out FastLED at a pinned release tag (currently `3.10.3`) so
measurements are reproducible, then runs the bench and uploads the JSON
report as an artifact. Bumping the pin is a deliberate baseline event —
update both the workflow `ref:` and the sample-numbers table above in
lockstep.

There is no automatic CI gate on the warm timings yet — first capture a
stable cross-runner baseline, then a follow-up adds the threshold gate.
The `fastled-examples` job in `.github/workflows/bench-205.yml` runs on
every PR that touches the resolver crates, this bench, or the workflow
itself. CI checks out FastLED at a pinned release tag (currently
`3.10.3`) so measurements are reproducible, then runs the bench with
`--max-warm-ms 50` and uploads the JSON report as an artifact. The
threshold is the AC#5 enforcement gate (see "AC#5 enforcement" above);
a breach fails the job and blocks the PR. Bumping the FastLED pin is a
deliberate baseline event — update both the workflow `ref:` and the
sample-numbers table above in lockstep.

## Cross-links

Expand Down
89 changes: 83 additions & 6 deletions bench/fastled-examples/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,17 @@
//! ## CLI
//!
//! ```text
//! bench-fastled-examples [--json <path>]
//! bench-fastled-examples [--json <path>] [--max-warm-ms <f64>]
//! ```
//!
//! `--json <path>` writes a structured report alongside the stdout table
//! for diffing in PR comments.
//!
//! `--max-warm-ms <f64>` enforces AC#5: each example whose warm timing
//! exceeds the threshold causes the binary to exit 1 after the table is
//! printed. Wired in CI at 50 ms (`~25x` headroom over current ~1-2 ms
//! warm numbers — absorbs runner noise without false positives).
//!
//! Refs: #205 Phase 7 (AC#5), #218.

use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -86,7 +91,8 @@ fn main() {

fn run() -> Result<(), Box<dyn std::error::Error>> {
let args: Vec<String> = std::env::args().collect();
let json_out = parse_json_flag(&args);
let json_out = parse_path_flag(&args, "--json");
let max_warm_ms = parse_f64_flag(&args, "--max-warm-ms")?;

let fastled_dir = resolve_fastled_dir()?;
let fastled_src = fastled_dir.join("src");
Expand All @@ -109,6 +115,10 @@ fn run() -> Result<(), Box<dyn std::error::Error>> {
println!();
println!("- FastLED: `{}`", fastled_dir.display());
println!("- Framework lib set: {} synthetic libs", libraries.len());
match max_warm_ms {
Some(t) => println!("- Warm threshold: {t:.2} ms"),
None => println!("- Warm threshold: none"),
}
println!();
println!("| example | cold (ms) | warm (ms) | speedup | selected | hit |");
println!("|---|---:|---:|---:|---:|---|");
Expand Down Expand Up @@ -138,12 +148,38 @@ fn run() -> Result<(), Box<dyn std::error::Error>> {
rows.push(row);
}

// Collect breaches (after the table prints, so the row data is always visible).
let breaches: Vec<(String, f64)> = match max_warm_ms {
Some(t) => rows
.iter()
.filter(|r| r.warm_ms > t)
.map(|r| (r.example.clone(), r.warm_ms))
.collect(),
None => Vec::new(),
};

if let Some(path) = json_out {
write_json_report(&path, &fastled_dir, &rows)?;
write_json_report(&path, &fastled_dir, &rows, max_warm_ms, &breaches)?;
println!();
println!("JSON report written to `{}`", path.display());
}

if let Some(t) = max_warm_ms {
if !breaches.is_empty() {
let summary: Vec<String> = breaches
.iter()
.map(|(name, ms)| format!("{name} ({ms:.2} ms)"))
.collect();
return Err(format!(
"AC#5 warm threshold breached: {} example(s) exceeded {:.2} ms: {}",
breaches.len(),
t,
summary.join(", ")
)
.into());
}
}

Ok(())
}

Expand Down Expand Up @@ -212,6 +248,8 @@ fn write_json_report(
path: &Path,
fastled_dir: &Path,
rows: &[Row],
max_warm_ms: Option<f64>,
breaches: &[(String, f64)],
) -> Result<(), Box<dyn std::error::Error>> {
let entries: Vec<_> = rows
.iter()
Expand All @@ -225,8 +263,19 @@ fn write_json_report(
})
})
.collect();
let breach_entries: Vec<_> = breaches
.iter()
.map(|(name, ms)| {
serde_json::json!({
"example": name,
"warm_ms": ms,
})
})
.collect();
let body = serde_json::json!({
"fastled_dir": fastled_dir.to_string_lossy(),
"max_warm_ms": max_warm_ms,
"breaches": breach_entries,
"rows": entries,
});
if let Some(parent) = path.parent() {
Expand All @@ -236,19 +285,47 @@ fn write_json_report(
Ok(())
}

fn parse_json_flag(args: &[String]) -> Option<PathBuf> {
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
}
Comment on lines +288 to 300
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


fn parse_f64_flag(args: &[String], flag: &str) -> Result<Option<f64>, Box<dyn std::error::Error>> {
let prefix = format!("{flag}=");
let mut iter = args.iter().skip(1);
while let Some(arg) = iter.next() {
let raw = if arg == flag {
match iter.next() {
Some(v) => v.as_str(),
None => return Err(format!("{flag} requires a value").into()),
}
} else if let Some(rest) = arg.strip_prefix(prefix.as_str()) {
rest
} else {
continue;
};
let parsed: f64 = raw
.parse()
.map_err(|e| format!("{flag} expects a number, got {raw:?}: {e}"))?;
if !parsed.is_finite() || parsed < 0.0 {
return Err(
format!("{flag} must be a non-negative finite number, got {parsed}").into(),
);
}
return Ok(Some(parsed));
}
Ok(None)
}

/// Read `FASTLED_DIR` from the environment. No fallback default: the
/// value depends on the host (CI uses a workspace-relative checkout,
/// developers use whatever convention they like) and silently
Expand Down
Loading