perf(build): extend watch-set fingerprint fast-path to AVR orchestrator#136
perf(build): extend watch-set fingerprint fast-path to AVR orchestrator#136
Conversation
📝 WalkthroughWalkthroughExtracted fast-path warm-build logic into a shared Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes involve substantial refactoring across multiple orchestrators, introduce a new architectural module with intricate fingerprint/watch/zccache logic, and alter persistence/validation mechanisms. Understanding the interaction between shared fast-path helpers, metadata hashing, artifact verification, and platform-specific callbacks across both AVR and ESP32 paths requires careful review. Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Extract the ESP32 no-op build fast-path check into a shared `build_fingerprint::fast_path` module and wire it into the AVR orchestrator. Warm `fbuild build tests/platform/uno -e uno --quick` drops from ~119 ms to ~75 ms (median of 8 runs) because the fast path returns before source scan, compile, and link. Refs #121. ESP32 side is a behaviour-preserving refactor: - `fast_path_check()` performs the same metadata / artifact / zccache / watch-set-stamp checks in the same order. - ESP32-specific compile-db-parity check rides the helper via a closure (`extra_artifact_ok`), so nothing about ESP32 fast-path semantics changes. - `FAST_PATH_EXTENSIONS` / `FAST_PATH_EXCLUDES` move into `build_fingerprint::fast_path` so both orchestrators share a single source of truth. AVR side adds the fast path end-to-end: - `AvrFingerprintMetadata` captures every field `AvrCompiler` / `AvrLinker` reads off `BoardConfig` (mcu, f_cpu, extra_flags, upload_protocol, etc.) plus toolchain / framework install paths. - Fast-path check lives after `ensure_installed` so the hashed paths reflect the real on-disk location. - Post-build, the orchestrator persists `build_fingerprint.json` and marks each watch in zccache so subsequent warm builds hit the helper. Follow-up PRs will extend this to Teensy / RP2040 / STM32 — they're the obvious next candidates now that the helper is in place. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zackees
force-pushed
the
perf/avr-fast-path-check
branch
from
761fd42 to
5666674
Compare
|
Rebased onto main to resolve conflicts with #128 ( All 6 conflict regions in No overlap with #134's P0 containment fix (that change lives in Verified locally:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fbuild-build/src/esp32/orchestrator.rs (1)
287-291:⚠️ Potential issue | 🟡 MinorRecompute the watch set before persisting the fingerprint.
fingerprint_watchesis collected before dependency libraries can materializebuild_dir/libs, but the same early list is later saved and marked in zccache. On the first build wherelibs/is created, the persisted hash can omitdep_libs, causing the next warm build to miss unnecessarily.♻️ Proposed adjustment
// 15. Size reporting + result assembly + let fingerprint_watches = collect_fast_path_watches(build_dir, ¶ms.project_dir); let persisted_fingerprint = PersistedBuildFingerprint { version: BUILD_FINGERPRINT_VERSION, metadata_hash: metadata_hash.clone(), file_set_hash: match hash_watch_set_stamps_cached( &fingerprint_watches,Also applies to: 1209-1234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fbuild-build/src/esp32/orchestrator.rs` around lines 287 - 291, fingerprint_watches is being collected too early (before dependency libraries materialize in build_dir/libs) which lets the persisted fingerprint omit dep_libs; to fix, move/recompute the call to collect_fast_path_watches so it runs immediately before you persist the fingerprint and mark it in zccache (i.e., after the step that materializes dependency libs), replacing the earlier use of the precomputed fingerprint_watches from build_fingerprint_path; update references around build_fingerprint_path, collect_fast_path_watches, and the zccache persist/mark code so the saved fingerprint uses the recomputed watch set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fbuild-build/src/avr/orchestrator.rs`:
- Around line 177-180: The initial fingerprint_watches are collected too early
(before run_sequential_build_with_libs may create build_dir/libs) causing stale
watches; fix by cloning ctx.build_dir before moving ctx into the pipeline and
then recomputing fingerprint_watches via collect_fast_path_watches (re-run the
perf.phase("fp-watches-collect") block) after the build completes, and pass that
refreshed watch set into hash_watch_set_stamps_cached and
mark_fingerprint_success (also apply the same change in the other occurrence
around lines 324-352 where watches are used).
In `@crates/fbuild-build/src/build_fingerprint/fast_path.rs`:
- Around line 41-44: The FAST_PATH_EXTENSIONS constant in fast_path.rs is
missing "ini", so PlatformIO config changes aren't tracked; update the
FAST_PATH_EXTENSIONS slice (symbol: FAST_PATH_EXTENSIONS) to include "ini" among
the extensions so .ini files are treated as inputs and cause the fast-path to
invalidate when they change.
---
Outside diff comments:
In `@crates/fbuild-build/src/esp32/orchestrator.rs`:
- Around line 287-291: fingerprint_watches is being collected too early (before
dependency libraries materialize in build_dir/libs) which lets the persisted
fingerprint omit dep_libs; to fix, move/recompute the call to
collect_fast_path_watches so it runs immediately before you persist the
fingerprint and mark it in zccache (i.e., after the step that materializes
dependency libs), replacing the earlier use of the precomputed
fingerprint_watches from build_fingerprint_path; update references around
build_fingerprint_path, collect_fast_path_watches, and the zccache persist/mark
code so the saved fingerprint uses the recomputed watch set.
🪄 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: ff6494b3-f9e9-4e4a-99f1-81ac64f56229
📒 Files selected for processing (5)
crates/fbuild-build/src/avr/orchestrator.rscrates/fbuild-build/src/build_fingerprint/README.mdcrates/fbuild-build/src/build_fingerprint/fast_path.rscrates/fbuild-build/src/build_fingerprint/mod.rscrates/fbuild-build/src/esp32/orchestrator.rs
| let fingerprint_watches = { | ||
| let _g = perf.phase("fp-watches-collect"); | ||
| collect_fast_path_watches(build_dir, ¶ms.project_dir) | ||
| }; |
There was a problem hiding this comment.
Refresh watches after the build before saving the AVR fingerprint.
build_dir/libs may not exist when fingerprint_watches is first collected, but it can be created by run_sequential_build_with_libs. Persisting the early watch set means the first warm build after dependency resolution can miss because it now sees an extra dep_libs watch.
If you recompute here after ctx is moved into the pipeline, clone ctx.build_dir before the move and use the refreshed watch set for both hash_watch_set_stamps_cached and mark_fingerprint_success.
Also applies to: 324-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-build/src/avr/orchestrator.rs` around lines 177 - 180, The
initial fingerprint_watches are collected too early (before
run_sequential_build_with_libs may create build_dir/libs) causing stale watches;
fix by cloning ctx.build_dir before moving ctx into the pipeline and then
recomputing fingerprint_watches via collect_fast_path_watches (re-run the
perf.phase("fp-watches-collect") block) after the build completes, and pass that
refreshed watch set into hash_watch_set_stamps_cached and
mark_fingerprint_success (also apply the same change in the other occurrence
around lines 324-352 where watches are used).
| pub const FAST_PATH_EXTENSIONS: &[&str] = &[ | ||
| "a", "bin", "c", "cc", "cpp", "csv", "elf", "h", "hh", "hpp", "ino", "json", "ld", "lds", "py", | ||
| "s", "S", "txt", | ||
| ]; |
There was a problem hiding this comment.
Include platformio.ini in the watched project inputs.
The shared project watch ignores .ini files, but PlatformIO env changes like build_flags, lib_deps, and filters can affect the produced firmware without being fully represented in the metadata hash. Add ini so config-only changes invalidate the fast path instead of reusing stale artifacts.
🐛 Proposed fix
pub const FAST_PATH_EXTENSIONS: &[&str] = &[
- "a", "bin", "c", "cc", "cpp", "csv", "elf", "h", "hh", "hpp", "ino", "json", "ld", "lds", "py",
- "s", "S", "txt",
+ "a", "bin", "c", "cc", "cpp", "csv", "elf", "h", "hh", "hpp", "ini", "ino", "json", "ld",
+ "lds", "py", "s", "S", "txt",
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub const FAST_PATH_EXTENSIONS: &[&str] = &[ | |
| "a", "bin", "c", "cc", "cpp", "csv", "elf", "h", "hh", "hpp", "ino", "json", "ld", "lds", "py", | |
| "s", "S", "txt", | |
| ]; | |
| pub const FAST_PATH_EXTENSIONS: &[&str] = &[ | |
| "a", "bin", "c", "cc", "cpp", "csv", "elf", "h", "hh", "hpp", "ini", "ino", "json", "ld", | |
| "lds", "py", "s", "S", "txt", | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/fbuild-build/src/build_fingerprint/fast_path.rs` around lines 41 - 44,
The FAST_PATH_EXTENSIONS constant in fast_path.rs is missing "ini", so
PlatformIO config changes aren't tracked; update the FAST_PATH_EXTENSIONS slice
(symbol: FAST_PATH_EXTENSIONS) to include "ini" among the extensions so .ini
files are treated as inputs and cause the fast-path to invalidate when they
change.
1 participant
Summary
Refs #121.
Extract the ESP32 no-op build fast-path into
build_fingerprint::fast_pathand wire it into the AVR orchestrator. Warm builds oftests/platform/unodrop from ~119 ms → ~75 ms wall-clock (median of 8 consecutive runs on Windows 10, daemon fully warm) because the fast path returns before source scan, compile, and link.The ESP32 side is a behaviour-preserving refactor — same metadata / artifact / zccache / watch-set-stamp checks in the same order; ESP32's compile-db-parity requirement rides the helper via
extra_artifact_ok.Warm-build timings:
fbuild build tests/platform/uno -e uno --quickMeasured via a Python harness capturing wall-clock around 10 back-to-back
subprocess.runcalls (median of last 8 after 2 warm-up iterations). Daemon kept running across all samples.With
FBUILD_PERF_LOG=1on the warm path post-change:The
--quickbuild log now containsNo-op fingerprint matched; reusing existing AVR artifacts.on warm rebuilds, confirming the fast path fires.The 50 ms target in the issue bakes in essentially zero CLI + daemon-handshake overhead; the pure server-side orchestrator time is ~50–60 ms here. The remainder is CLI startup / HTTP round-trip.
What changed
crates/fbuild-build/src/build_fingerprint/fast_path.rs—FastPathInputs,FastPathHit,fast_path_check, plus the sharedFAST_PATH_EXTENSIONS/FAST_PATH_EXCLUDESconstants andfast_path_watchhelper.build_fingerprint.rs→build_fingerprint/mod.rs— file-to-directory conversion for the new submodule.fast_path_checkcall. ESP32'scompile_db_is_currentfreshness requirement is threaded through the newextra_artifact_okhook so behaviour is preserved bit-for-bit.AvrFingerprintMetadata,collect_fast_path_watches, and a post-build persistence step that writesbuild_fingerprint.json+ marks zccache watches.fast_path::tests— cache hit, missing fingerprint, changed source file, mismatched metadata hash, missing artifact, and theextra_artifact_okescape hatch.Follow-up orchestrators
With the helper in place, wiring the fast path into the remaining platforms should be a small patch each:
crates/fbuild-build/src/teensy/orchestrator.rscrates/fbuild-build/src/rp2040/orchestrator.rscrates/fbuild-build/src/stm32/orchestrator.rsEach follows the AVR template: define a platform-specific
FingerprintMetadata, collect project + resolved-lib watches, gate thefast_path_checkon!compiledb_only && !symbol_analysis, and persist the fingerprint post-build. Scoped out of this PR per the issue.Test plan
uv run cargo clippy --workspace --all-targets -- -D warnings— cleanuv run cargo test -p fbuild-build— 460 unit + 2 integration tests pass (including 6 newfast_path::tests)fbuild build tests/platform/uno -e uno --quickemits "No-op fingerprint matched" and returns in ~75 msfbuild build tests/platform/esp32dev -e esp32dev --target compiledbsucceeds — ESP32 refactor didn't break its compile-db code path🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Documentation