Skip to content

feat: publish nightly build benchmark#1145

Merged
zackees merged 2 commits into
mainfrom
feat/build-benchmark-site
Jul 22, 2026
Merged

feat: publish nightly build benchmark#1145
zackees merged 2 commits into
mainfrom
feat/build-benchmark-site

Conversation

@zackees

@zackees zackees commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • benchmark Arduino CLI, PlatformIO, and fbuild against one pinned Arduino Uno Blink fixture
  • publish median cold/warm timings as an overlaid SVG, HTML report, latest JSON, manifest, and rolling JSONL history
  • force-publish a one-commit benchmark-stats branch nightly and deploy the same snapshot through GitHub Pages
  • embed the generated chart and stable machine-readable manifest link in the main README

Validation

  • soldr cargo test -p fbuild-bench-fastled-examples --bin bench-build-comparison
  • soldr cargo clippy -p fbuild-bench-fastled-examples --bin bench-build-comparison -- -D warnings
  • actionlint .github/workflows/benchmark-build-comparison.yml
  • uv run --no-project python ci/check_workspace_crates.py
  • real Arduino CLI, PlatformIO, and current-source fbuild smoke builds of bench/blink
  • generated-site smoke test covering manifest/latest/history/HTML/SVG output
  • clud-review: clean (Rust, CI, docs, config, fixture)

The repository Pages build type has already been switched from legacy to GitHub Actions so the deploy job can publish after merge.

Summary by CodeRabbit

  • New Features

    • Added nightly and on-demand build performance comparisons for Arduino Blink builds across three toolchains.
    • Published benchmark charts, summaries, and historical results to a GitHub Pages site.
    • Added a shared Arduino Uno Blink benchmark fixture and generated performance reports.
  • Documentation

    • Added documentation for running benchmarks locally, interpreting results, and accessing published performance data.
    • Added a build performance section and benchmark status badge to the main README.
  • Chores

    • Updated ignore rules for generated benchmark and build artifacts.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@zackees, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 42 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: acb856c0-73da-472e-b4a1-4ad9cc8118d7

📥 Commits

Reviewing files that changed from the base of the PR and between ec5276f and 88876a0.

📒 Files selected for processing (3)
  • .github/workflows/benchmark-build-comparison.yml
  • bench/blink/README.md
  • bench/fastled-examples/src/build_comparison.rs
📝 Walkthrough

Walkthrough

This change adds a shared Arduino Uno Blink fixture, a Rust runner comparing Arduino CLI, PlatformIO, and fbuild build times, generated benchmark artifacts, and scheduled GitHub Actions publication to benchmark-stats and GitHub Pages.

Changes

Build Comparison Benchmark

Layer / File(s) Summary
Blink fixture and benchmark binary wiring
bench/blink/*, bench/fastled-examples/Cargo.toml, bench/fastled-examples/src/README.md, bench/README.md
Adds the Arduino sketch, PlatformIO Uno environment, bench-build-comparison binary target, and benchmark documentation.
Cross-tool measurement harness
bench/fastled-examples/src/build_comparison.rs
Runs cold and warm builds across three toolchains, records timings, computes medians and speedups, validates options, and logs command execution.
Benchmark artifact generation and validation
bench/fastled-examples/src/build_comparison.rs
Writes JSON, SVG, HTML, and history artifacts, renders report metadata, and tests median calculations, chart output, and bounded history.
Scheduled CI and Pages publication
.github/workflows/benchmark-build-comparison.yml, .github/workflows/README.md, README.md, .gitignore
Adds scheduled/manual benchmark execution, artifact publication to benchmark-stats, GitHub Pages deployment, documentation links, and local artifact ignore rules.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant benchmark as benchmark job
  participant runner as bench-build-comparison
  participant stats as benchmark-stats branch
  participant publish as publish job
  participant pages as GitHub Pages
  participant deploy as deploy-pages job
  benchmark->>runner: run cold and warm toolchain trials
  runner-->>benchmark: write benchmark site snapshot and log
  benchmark->>publish: upload benchmark-stats artifact
  publish->>stats: force-push generated snapshot
  publish->>pages: upload Pages artifact
  deploy->>pages: deploy uploaded benchmark site
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main change: adding a nightly build benchmark and publishing its results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/build-benchmark-site

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (2)
bench/fastled-examples/src/build_comparison.rs (2)

408-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a unit test for remove_dir_within's traversal guard.

This is the one path-safety-critical function in the file (guarding against removing a directory outside the repo root), but it has no dedicated test — only exercised indirectly. Worth a direct test covering target == root, a sibling path outside root, and a valid nested path.

🤖 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/build_comparison.rs` around lines 408 - 421, Add a
focused unit test for remove_dir_within covering rejection of target == root,
rejection of a sibling path outside root, and successful removal of a valid
nested directory. Use temporary directories and verify the nested target is
removed while guarded paths remain unaffected.

191-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Bundle the repeated build-context parameters.

measure_tool, clean_tool, and timed_build each thread the same 6-7 parameters (options, repo_root, project_dir, fbuild, arduino_build_dir, log), forcing #[allow(clippy::too_many_arguments)] on measure_tool. Bundling these into a small BuildContext struct would remove the lint override and reduce call-site noise.

♻️ Sketch of the refactor
+struct BuildContext<'a> {
+    options: &'a Options,
+    repo_root: &'a Path,
+    project_dir: &'a Path,
+    fbuild: &'a Path,
+    arduino_build_dir: &'a Path,
+}
+
-fn measure_tool(
-    kind: ToolKind,
-    version: &str,
-    options: &Options,
-    repo_root: &Path,
-    project_dir: &Path,
-    fbuild: &Path,
-    arduino_build_dir: &Path,
-    log: &mut File,
-) -> AppResult<ToolResult> {
+fn measure_tool(kind: ToolKind, version: &str, ctx: &BuildContext, log: &mut File) -> AppResult<ToolResult> {
🤖 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/build_comparison.rs` around lines 191 - 357,
Introduce a small BuildContext struct containing options, repo_root,
project_dir, fbuild, arduino_build_dir, and log, then update measure_tool,
clean_tool, and timed_build to accept and access this context instead of
threading the individual parameters. Adjust all call sites accordingly and
remove the #[allow(clippy::too_many_arguments)] override from measure_tool.
🤖 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 @.github/workflows/benchmark-build-comparison.yml:
- Line 36: Update the actions/checkout step in the benchmark build comparison
workflow to set persist-credentials to false. Leave the existing checkout
configuration unchanged otherwise.
- Around line 139-146: Update the publish job’s if condition to compare
github.ref against the literal default branch reference refs/heads/main, while
retaining the successful benchmark requirement. Remove the dependency on
github.event.repository.default_branch for the scheduled publish gate.

In `@bench/blink/platformio.ini`:
- Around line 1-7: The benchmark currently pins only the PlatformIO board
platform, not the Arduino AVR core used by the comparison build. In
bench/blink/platformio.ini, align platform_packages/framework-arduino-avr with
the core revision from arduino:avr@1.8.8; alternatively, update
bench/blink/README.md to soften its “exact same target” claim if exact core
alignment is not possible.

---

Nitpick comments:
In `@bench/fastled-examples/src/build_comparison.rs`:
- Around line 408-421: Add a focused unit test for remove_dir_within covering
rejection of target == root, rejection of a sibling path outside root, and
successful removal of a valid nested directory. Use temporary directories and
verify the nested target is removed while guarded paths remain unaffected.
- Around line 191-357: Introduce a small BuildContext struct containing options,
repo_root, project_dir, fbuild, arduino_build_dir, and log, then update
measure_tool, clean_tool, and timed_build to accept and access this context
instead of threading the individual parameters. Adjust all call sites
accordingly and remove the #[allow(clippy::too_many_arguments)] override from
measure_tool.
🪄 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: 201f395c-45b7-4195-8044-f98680039bb4

📥 Commits

Reviewing files that changed from the base of the PR and between c628ca5 and ec5276f.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • .github/workflows/README.md
  • .github/workflows/benchmark-build-comparison.yml
  • .gitignore
  • README.md
  • bench/README.md
  • bench/blink/README.md
  • bench/blink/blink.ino
  • bench/blink/platformio.ini
  • bench/fastled-examples/Cargo.toml
  • bench/fastled-examples/src/README.md
  • bench/fastled-examples/src/build_comparison.rs

Comment thread .github/workflows/benchmark-build-comparison.yml
Comment thread .github/workflows/benchmark-build-comparison.yml
Comment thread bench/blink/platformio.ini
@zackees
zackees merged commit 96a128e into main Jul 22, 2026
89 of 96 checks passed
@zackees
zackees deleted the feat/build-benchmark-site branch July 22, 2026 19:40
@fastled-project-sync fastled-project-sync Bot moved this to Triage in FastLED Tracker Jul 23, 2026
zackees added a commit that referenced this pull request Jul 23, 2026
… gate

The Formatting and Check workflows have been red on main since #1145:
- four files (two from this branch, two pre-existing: board/loaders.rs,
  ch32v_core.rs) carry diffs under the pinned 1.94.1 formatter. Local
  cargo-fmt --all silently skips the daemon handler files on this
  checkout, so they were formatted with direct invocations.
- the #1150 health-classification helpers in fbuild-serial have no
  non-Windows, non-test caller, which RUSTFLAGS=-D warnings promotes to
  a hard error on the ubuntu/macos Check jobs. Annotate them
  cfg_attr(not(windows), allow(dead_code)) — they are fed by the
  Windows PnP enumeration and unit-tested cross-platform.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zackees added a commit that referenced this pull request Jul 23, 2026
…quisition (#1153)

* fix(rp2040): consume Windows port health in deploy selection and reacquisition

A retained CM_PROB_PHANTOM devnode whose serial still matches the board
could be selected pre-flash, touched at 1200 baud, and returned as the
post-flash deploy port. Health from #1146/#1150 now gates every step:

- rp2040_target: select_cdc_candidate returns a health-eligible
  PicoCdcPort (phantom/present-problem records are never selected);
  resolve_requested_runtime_target fails an explicit selector that
  matches a known-unhealthy record, with health/problem/instance detail
- rp2040: the post-flash wait adds a bounded openability probe seam;
  timeout diagnostics carry the last open error and phantom-aware
  BOOT/RESET recovery guidance; Deployer::owns_post_flash_port_discovery
  marks the RP2040 port as authoritative
- daemon: recovery/monitor port resolution no longer substitutes the
  requested pre-flash name when the deployer owns port discovery
  (deploy_port_str.or(result.port) inverted the trust order); flash
  success with an unrecovered CDC now surfaces the deployer's recovery
  diagnostic instead of discarding it; choose_deploy_port partitions
  known-unhealthy RP2040 records out of auto-selection with a
  diagnostic warning

Closes #1147

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): satisfy pinned formatter tree-wide and non-Windows dead-code gate

The Formatting and Check workflows have been red on main since #1145:
- four files (two from this branch, two pre-existing: board/loaders.rs,
  ch32v_core.rs) carry diffs under the pinned 1.94.1 formatter. Local
  cargo-fmt --all silently skips the daemon handler files on this
  checkout, so they were formatted with direct invocations.
- the #1150 health-classification helpers in fbuild-serial have no
  non-Windows, non-test caller, which RUSTFLAGS=-D warnings promotes to
  a hard error on the ubuntu/macos Check jobs. Annotate them
  cfg_attr(not(windows), allow(dead_code)) — they are fed by the
  Windows PnP enumeration and unit-tested cross-platform.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): mark registry-backed boards doctests no_run

board_hint, vcom_for_env, and family_for_vid_pid resolve through the
FastLED/boards registry at runtime; on doctest hosts (local and CI)
that cache is absent, so the three examples panicked on every Check
job since the vendored-table migration. no_run keeps them
compile-checked without requiring a populated registry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): annotate riscv multilib GCC probe for the direct-spawn lint

ci/find_direct_subprocess.py --fail has flagged the ch32v multilib
probe since it landed, keeping the Check jobs red. The spawn is a
short synchronous -print-multi-directory capability probe; mark it
allow-direct-spawn rather than routing a one-flag probe through the
subprocess facade.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): drop embedded WCH ISP VID/PID from the wchisp failure message

The USB VID/PID catalogue-literal guard has flagged this user-facing
string since the wchisp deployer landed, keeping Check (ubuntu) red.
Identity data belongs to the FastLED/boards registry; point the user
at 'fbuild port scan' instead of embedding 4348/1a86 literals.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): apply nightly import ordering in ban_raw_subprocess dylint

The Dylint crate-formatting gate (nightly style edition) has wanted
these two use-statement reorders since the lint landed; apply them so
the gate goes green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): apply nightly import ordering in ban_raw_path_prefix_compare dylint

Same nightly-2026-03-26 style-edition reorders as ban_raw_subprocess;
verified with the crate-pinned formatter that both format-checked
dylint crates are now clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): allowlist riscv toolchain GCC probe in ban_raw_subprocess dylint

Companion to the Python-lint marker: the workspace dylint pass wants
its own allowlist.txt entry for the same short synchronous
-print-multi-directory capability probe. ci/check_dylint_allowlists.py
passes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): allowlist esp32 framework-library cache in ban_raw_path_prefix_compare

WalkDir-derived paths are stripped against the exact root spelling they
were enumerated from (same-normalized by construction) and feed a
content hash, not a path-keyed cache lookup — the lint's own stated
exemption. Pre-existing debt unmasked once the earlier Dylint gate
failures were fixed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): force rebuild of ban_raw_path_prefix_compare after allowlist edit

The previous round still flagged the freshly-allowlisted esp32
framework-library cache: the workspace dylint step served a compiled
lint from the restored build cache, ignoring the allowlist-only edit.
Touch the lint source (documenting the trap) so the fingerprint moves.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): refresh stale dylint @toolchain aliases after rebuilds

cargo-dylint loads the <name>@<toolchain> library copy, but the alias
loop only created it when absent. With target caching, a restored stale
alias therefore pinned every lint to its first-ever compiled allowlist:
rebuilt bare libraries (allowlist/source edits) were never loaded, which
is why ban_raw_path_prefix_compare kept flagging the freshly-allowlisted
esp32 framework-library cache. Overwrite the alias when the bare library
is newer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

1 participant