perf(esp32): compile the sketch concurrently with the framework core - #1545
Conversation
|
Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: FastLED/fbuild/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
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 |
The ESP32 orchestrator ran the compile phases strictly sequentially: compile-core-variant -> core-cache-store -> compile-sketch -> link. The sketch translation unit therefore had its own slot on the critical path and nothing else to overlap with, so a cold build paid its full duration as pure added wall time. PlatformIO compiles the sketch together with the core files and pays nothing extra for it. Compile both against one shared job gate. The sketch is submitted first and the gate has far more permits than the first wave needs, so its single task starts alongside the core fan-out instead of being scheduled after it. Sharing one semaphore is also what keeps the two phases from multiplying the job budget: the region still runs at most `jobs` compilers, where giving each phase its own pool would permit two. That budget is not the binding constraint today - measured on 16 cores, this build peaks at 10-12 concurrent compilers even at the default `jobs = ncpu * 2`, and `-j 16` vs `-j 32` is a wash (3.56s vs 3.61s median over 3 interleaved cold trials) - but it is what keeps it that way now that the phases overlap. One `PhaseGuard` covers the region both phases share, so `compile-core-variant` now spans the concurrent region and `compile-sketch` is recorded separately as the sketch's own span rather than as a serial wall-clock phase. Measured on bench/blink -e esp32s3 (16-core Linux), 5 interleaved cold trials, median, every trial a genuine framework-core cache miss with the same cache key on both sides: wall 3.66s -> 3.38s (-7.7%) `firmware.bin` / `firmware.elf` are byte-identical to the pre-change build - this moves work, it does not change what is compiled. Refs #1537
…dule
`build.rs` sits at exactly the 1000-LOC gate on main, so the previous commit
pushed it over and failed `Reject .rs files over 1000 LOC`. Extract the
core+sketch concurrent-compile block into `compile_phases.rs`, which is what
the directory layout is already for ("the orchestrator is split across sibling
files in this directory to keep each one under the 1000-LOC gate").
Pure move: same shared gate, same submit order, same phase guards. build.rs
goes 1030 -> 993 lines.
…d_pathbuf `dylints/ban_std_pathbuf` denies naming `std::path::PathBuf` in a new file, and `compile_phases.rs` is new. The compile engine's API takes exactly that type, so the paths are now materialised inside the two async blocks, where the element type is inferred from the engine call, and the boundary is generic over `AsRef<Path>` so callers pass their existing vectors unchanged. Verified with the same command CI runs: `env -u RUSTUP_TOOLCHAIN soldr dylint --all -- --package fbuild-build-esp --all-targets --target x86_64-unknown-linux-gnu` exits 0.
1df08e9 to
3b0e51a
Compare
|
Rebased onto Also added a commit fixing the Re-verified after the rebase + refactor: cold build succeeds and |
1 participant
Problem
Part of #1537 (cause 2). The ESP32 orchestrator ran the compile phases strictly sequentially:
The sketch TU therefore had its own slot on the critical path with nothing to overlap with, so a cold build paid its full duration as pure added wall time —
compile-sketchmeasures 374 ms, matching thecompile-sketch: 589msin the issue. PlatformIO compiles the sketch alongside the core files and pays nothing extra.Fix
Compile both against one shared job gate.
compile_sources_parallel_sharedalready exists for exactly this (#1468); it just was not used here.The sketch future is submitted first and the gate has far more permits than the first wave needs, so its single task starts alongside the core fan-out instead of being scheduled after it.
Sharing one semaphore is also what keeps the two phases from multiplying the job budget: the region still runs at most
jobscompilers, where giving each phase its own pool would permit two.One
PhaseGuardcovers the region both phases now share, socompile-core-variantspans the concurrent region andcompile-sketchis recorded separately as the sketch's own span rather than as a serial wall-clock phase. (The two spans deliberately overlap, so Σphases now exceedstotal_msfor these two entries.)On the oversubscription claim in the issue
Investigated and did not change the
ncpu * 2default, because it is not causing oversubscription on this workload:-j 32(tight 10 ms sampler over the whole build)-j 6 / 12 / 16 / 32cold medians: 4.26 / 3.63 / 3.56 / 3.61 s — a plateau from 12 up, with the default already on it-j 32is not a regression, so lowering it would only risk regressing larger sketches that use the extra permits to hide zccache I/OThe shared gate in this PR is the part of "oversubscription" that actually needed fixing: it is what prevents the newly-concurrent phases from compounding the budget.
Verification
5 interleaved cold trials each, median, round-robined, daemon torn down per trial, and confirmed identical framework-core cache keys (
45cae7382e31) on both sides so the comparison is apples-to-apples:compile-core-variantcompile-sketchfirmware.bin/firmware.elfare byte-identical to the pre-change build (abe3b6ae285fe8f6/1a1e79faf9b22fbb) — this moves work, it does not change what is compiled.soldr cargo test -p fbuild-build-esp— 133 passedsoldr cargo clippy -p fbuild-build-esp --all-targetscleansoldr cargo fmt --allappliedRefs #1537