From 55d63164293b76b059675b165e6d1f3f9a125407 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Mon, 28 Sep 2026 00:30:45 -0700 Subject: [PATCH 1/2] perf(esp32): cut SDK -I fan-out from 385 dirs to PlatformIO's curated list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ESP32-S3 cold builds pass 385 `-I` directories where PlatformIO passes 199. Same toolchain, same flags — fbuild even omits pio's `-ggdb`. Swapping only the include list moves a single translation unit ~41% (0.652s -> 0.462s, min of 3, one TU, same g++ invocation both sides), and the whole 47-TU build pays it, so this is the largest single term in the cold-build gap reported in FastLED/fbuild#1537. The fan-out comes from `get_sdk_include_dirs`'s fallback: when the SDK has no `flags/includes` (every arduino-esp32 2.x layout, e.g. the `espressif32@6.13.0` fixture, whose SDK lives at `tools/sdk/`), it reconstructs the list by scanning `include/` to a fixed depth. That scan is wrong in both directions: - too shallow: it misses leaves PlatformIO passes, so `bt/common/api/include/api`, `bt/host/bluedroid/api/include/api` and `lwip/port/esp32/include/arch` are absent and any header reached through them fails to resolve; - too greedy: it emits ~220 dirs PlatformIO never passes, including per-chip dirs for *other* chips (`soc/esp32`, `port/soc/esp32c3`, `esp_hw_pm/include/esp32s2`), component test dirs (`json/cJSON/tests`) and an `esp-dsp` fan-out. Every extra `-I` costs a failed path lookup on every unresolved `#include`, and these paths are deep, so the cost is real per TU. Those framework layouts do ship an authoritative list: the SCons builder PlatformIO itself uses for that exact framework version, `tools/platformio-build-.py`, whose `CPPPATH` block is the include list in PlatformIO's order. Parse it and use it, keeping the existing `flags/includes` path for the newer layout and the tree scan as the last resort. A parse that yields fewer than 20 entries is treated as a failure rather than trusted, so an upstream format change degrades to today's behaviour instead of silently truncating the include path. Entries computed from `env.BoardConfig()` (the flash/PSRAM variant dir, the core dir) are skipped; the caller already supplies those. Measured on bench/blink -e esp32s3: 385 -> 200 `-I` (PlatformIO: 199), with the previously-missing deep leaves now present, and the cold build compiles and links to the same firmware. Refs FastLED/fbuild#1537 --- .../src/library/esp32_framework/sdk_paths.rs | 95 +++++++++++++++++++ .../src/library/esp32_framework/tests.rs | 93 ++++++++++++++++++ 2 files changed, 188 insertions(+) diff --git a/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs b/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs index 94560046..1e45049e 100644 --- a/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs +++ b/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs @@ -2,12 +2,93 @@ //! shipped with the ESP32 Arduino framework. use std::cmp::Ordering; +use std::collections::HashSet; use std::path::{Path, PathBuf}; use super::Esp32Framework; use super::fs_utils::{collect_archive_files, scan_include_dirs_recursive}; use super::parsing::{parse_include_flags, split_defines}; +/// A parsed builder script yielding fewer entries than this is treated as a +/// failed parse. An upstream format change must degrade to the tree scan, +/// never to a silently truncated include path. +const MIN_PIO_CPPPATH_ENTRIES: usize = 20; + +/// Resolve a `join(FRAMEWORK_DIR, "tools", "sdk", ...)` call from the +/// framework's PlatformIO builder script into a path relative to the +/// framework root. Returns `None` when the call is anything other than plain +/// string literals (e.g. one computed from `env.BoardConfig()`). +fn pio_join_literals(line: &str) -> Option { + let mut path = PathBuf::new(); + let mut rest = line; + while let Some(start) = rest.find('"') { + let after = &rest[start + 1..]; + let end = after.find('"')?; + path.push(&after[..end]); + rest = &after[end + 1..]; + } + if path.as_os_str().is_empty() { + return None; + } + Some(path) +} + +/// Read the include list out of the framework's own PlatformIO builder script +/// (`tools/platformio-build-.py`). +/// +/// SDK layouts that predate `flags/includes` (arduino-esp32 2.x) have no +/// machine-readable include list, but they do ship the SCons builder that +/// PlatformIO uses for exactly this framework version, and its `CPPPATH` +/// block is that list, in PlatformIO's order. +/// +/// Reconstructing the list by scanning the tree instead is wrong in both +/// directions: the depth cap misses leaves PlatformIO passes (e.g. +/// `bt/common/api/include/api`, `lwip/port/esp32/include/arch`) while still +/// emitting hundreds of dirs PlatformIO never passes. Every extra `-I` costs +/// a failed path lookup on every unresolved `#include`, which measured ~41% +/// per translation unit on ESP32-S3 (FastLED/fbuild#1537). +/// +/// Returns `None` when the script is absent or does not parse, so the caller +/// can fall back to the tree scan. +fn parse_pio_cpppath(root: &Path, mcu: &str) -> Option> { + let script = root + .join("tools") + .join(format!("platformio-build-{mcu}.py")); + let content = std::fs::read_to_string(&script).ok()?; + + let mut dirs = Vec::new(); + let mut in_block = false; + for line in content.lines() { + let trimmed = line.trim(); + if !in_block { + if trimmed.starts_with("CPPPATH=[") || trimmed.starts_with("CPPPATH = [") { + in_block = true; + } + continue; + } + if trimmed.starts_with(']') { + break; + } + if !trimmed.contains("join(FRAMEWORK_DIR") || trimmed.contains("env.") { + continue; + } + if let Some(rel) = pio_join_literals(trimmed) { + let resolved = root.join(rel); + if resolved.exists() { + dirs.push(resolved); + } + } + } + + if dirs.len() < MIN_PIO_CPPPATH_ENTRIES { + return None; + } + + let mut seen = HashSet::new(); + dirs.retain(|dir| seen.insert(dir.clone())); + Some(dirs) +} + /// Get the SDK directory for a given MCU. /// /// Tries new layout (`tools/esp32-arduino-libs/{mcu}`) first, falls back to @@ -97,6 +178,20 @@ impl Esp32Framework { } } + // Old-layout SDK (arduino-esp32 2.x) with no `flags/includes`: use the + // include list from the framework's own PlatformIO builder script. + if let Some(mut dirs) = parse_pio_cpppath(&root, mcu) { + // The flash/PSRAM variant entry is computed from board config in + // the script, so the caller supplies it (as it does above). + if let Some(variant_dir) = sdk_memory_variant_dir(&sdk_dir, memory_type) { + let v_include = variant_dir.join("include"); + if v_include.exists() && !dirs.contains(&v_include) { + dirs.push(v_include); + } + } + return dirs; + } + // Fallback: recursively scan include/ subdirectories. // The 2.x framework (PlatformIO-compat) has deeply nested includes // under tools/sdk/{mcu}/include/ (e.g., freertos/include/freertos, diff --git a/crates/fbuild-library/src/library/esp32_framework/tests.rs b/crates/fbuild-library/src/library/esp32_framework/tests.rs index b0124b4e..0dc1fd33 100644 --- a/crates/fbuild-library/src/library/esp32_framework/tests.rs +++ b/crates/fbuild-library/src/library/esp32_framework/tests.rs @@ -442,3 +442,96 @@ fn test_split_defines_empty() { fn test_split_defines_single() { assert_eq!(split_defines("-DFOO=1"), vec!["-DFOO=1"]); } + +#[test] +fn old_sdk_uses_pio_builder_include_list_over_tree_scan() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + // A tree scan would find the decoy (a header dir under include/) and miss + // the deeply nested leaf that only the builder script names. + let decoy = root.join("tools/sdk/esp32s3/include/decoy/include"); + std::fs::create_dir_all(&decoy).unwrap(); + std::fs::write(decoy.join("decoy.h"), "\n").unwrap(); + + let leaf = root.join("tools/sdk/esp32s3/include/bt/common/api/include/api"); + std::fs::create_dir_all(&leaf).unwrap(); + std::fs::write(leaf.join("esp_bt.h"), "\n").unwrap(); + let deep = root.join("tools/sdk/esp32s3/include/lwip/port/esp32/include"); + std::fs::create_dir_all(&deep).unwrap(); + std::fs::write(deep.join("lwipopts.h"), "\n").unwrap(); + + std::fs::create_dir_all(root.join("tools")).unwrap(); + let mut script = String::from("env.Append(\n CPPPATH=[\n"); + for i in 0..25 { + let rel = format!("tools/sdk/esp32s3/include/comp{i}/include"); + std::fs::create_dir_all(root.join(&rel)).unwrap(); + std::fs::write(root.join(&rel).join("h.h"), "\n").unwrap(); + script.push_str(&format!(" join(FRAMEWORK_DIR, \"tools\", \"sdk\", \"esp32s3\", \"include\", \"comp{i}\", \"include\"),\n")); + } + script.push_str(" join(FRAMEWORK_DIR, \"tools\", \"sdk\", \"esp32s3\", \"include\", \"bt\", \"common\", \"api\", \"include\", \"api\"),\n"); + script.push_str(" join(FRAMEWORK_DIR, \"tools\", \"sdk\", \"esp32s3\", \"include\", \"lwip\", \"port\", \"esp32\", \"include\"),\n"); + script.push_str(" join(FRAMEWORK_DIR, \"tools\", \"sdk\", \"esp32s3\", env.BoardConfig().get(\"build.flash_mode\"), \"include\"),\n"); + script.push_str(" join(FRAMEWORK_DIR, \"cores\", env.BoardConfig().get(\"build.core\"))\n ],\n)\n"); + std::fs::write(root.join("tools/platformio-build-esp32s3.py"), script).unwrap(); + + let fw = Esp32Framework { + base: PackageBase::new( + "test", + "1.0", + "http://example.com", + "http://example.com", + None, + CacheSubdir::Platforms, + tmp.path(), + ), + install_dir: Some(tmp.path().to_path_buf()), + }; + + let dirs = fw.get_sdk_include_dirs("esp32s3", None); + assert!( + dirs.contains(&deep), + "deep leaf from builder script missing" + ); + assert!( + dirs.contains(&leaf), + "nested leaf from builder script missing" + ); + assert!( + !dirs.contains(&decoy), + "tree scan decoy leaked into the builder-script list" + ); + // Order follows the script, not a sort: the script names `bt` before `lwip`. + assert!(dirs.iter().position(|d| d == &leaf) < dirs.iter().position(|d| d == &deep)); +} + +#[test] +fn old_sdk_falls_back_to_tree_scan_when_builder_script_unparseable() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + let include = root.join("tools/sdk/esp32s3/include/efuse/include"); + std::fs::create_dir_all(&include).unwrap(); + std::fs::write(include.join("esp_efuse.h"), "\n").unwrap(); + std::fs::create_dir_all(root.join("tools")).unwrap(); + // Truncated/substituted CPPPATH that yields too few entries to trust. + std::fs::write( + root.join("tools/platformio-build-esp32s3.py"), + "env.Append(\n CPPPATH=[\n join(FRAMEWORK_DIR, \"cores\")\n ],\n)\n", + ) + .unwrap(); + + let fw = Esp32Framework { + base: PackageBase::new( + "test", + "1.0", + "http://example.com", + "http://example.com", + None, + CacheSubdir::Platforms, + tmp.path(), + ), + install_dir: Some(tmp.path().to_path_buf()), + }; + + let dirs = fw.get_sdk_include_dirs("esp32s3", None); + assert!(dirs.contains(&include), "tree-scan fallback did not run"); +} From 85a503915e56eaeb74703416e6f47da9e4a701d1 Mon Sep 17 00:00:00 2001 From: Zach Vorhies Date: Mon, 28 Sep 2026 00:56:47 -0700 Subject: [PATCH 2/2] fix(esp32): apply the builder-script entry threshold to distinct paths `parse_pio_cpppath` counted parsed entries before deduplicating them, so a script that repeated one entry enough times would clear `MIN_PIO_CPPPATH_ENTRIES` and then dedupe down to a single-directory include path - exactly the silent truncation that threshold exists to prevent. Dedupe first, then apply the threshold, so the count that decides trust is the number of distinct directories. Adds a regression test that pads the CPPPATH block with 30 copies of one entry and asserts the tree-scan fallback runs instead. --- .../src/library/esp32_framework/sdk_paths.rs | 8 +++- .../src/library/esp32_framework/tests.rs | 45 +++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs b/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs index 1e45049e..8461f464 100644 --- a/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs +++ b/crates/fbuild-library/src/library/esp32_framework/sdk_paths.rs @@ -80,12 +80,16 @@ fn parse_pio_cpppath(root: &Path, mcu: &str) -> Option> { } } + // Dedupe before applying the threshold: the count that matters is distinct + // paths, so a script that repeats one entry enough times cannot pass the + // guard and then dedupe down to a truncated include path. + let mut seen = HashSet::new(); + dirs.retain(|dir| seen.insert(dir.clone())); + if dirs.len() < MIN_PIO_CPPPATH_ENTRIES { return None; } - let mut seen = HashSet::new(); - dirs.retain(|dir| seen.insert(dir.clone())); Some(dirs) } diff --git a/crates/fbuild-library/src/library/esp32_framework/tests.rs b/crates/fbuild-library/src/library/esp32_framework/tests.rs index 0dc1fd33..f7d37287 100644 --- a/crates/fbuild-library/src/library/esp32_framework/tests.rs +++ b/crates/fbuild-library/src/library/esp32_framework/tests.rs @@ -535,3 +535,48 @@ fn old_sdk_falls_back_to_tree_scan_when_builder_script_unparseable() { let dirs = fw.get_sdk_include_dirs("esp32s3", None); assert!(dirs.contains(&include), "tree-scan fallback did not run"); } + +#[test] +fn old_sdk_rejects_builder_script_padded_with_duplicate_entries() { + let tmp = tempfile::TempDir::new().unwrap(); + let root = tmp.path(); + // A dir only the tree scan would find, used to prove the fallback ran. + let scanned = root.join("tools/sdk/esp32s3/include/efuse/include"); + std::fs::create_dir_all(&scanned).unwrap(); + std::fs::write(scanned.join("esp_efuse.h"), "\n").unwrap(); + + let dup = root.join("tools/sdk/esp32s3/include/only/include"); + std::fs::create_dir_all(&dup).unwrap(); + std::fs::write(dup.join("only.h"), "\n").unwrap(); + + std::fs::create_dir_all(root.join("tools")).unwrap(); + // Same entry repeated past the minimum: distinct-path count is 1, so this + // must not be trusted even though the line count clears the threshold. + let mut script = String::from("env.Append(\n CPPPATH=[\n"); + for _ in 0..30 { + script.push_str( + " join(FRAMEWORK_DIR, \"tools\", \"sdk\", \"esp32s3\", \"include\", \"only\", \"include\"),\n", + ); + } + script.push_str(" ],\n)\n"); + std::fs::write(root.join("tools/platformio-build-esp32s3.py"), script).unwrap(); + + let fw = Esp32Framework { + base: PackageBase::new( + "test", + "1.0", + "http://example.com", + "http://example.com", + None, + CacheSubdir::Platforms, + tmp.path(), + ), + install_dir: Some(tmp.path().to_path_buf()), + }; + + let dirs = fw.get_sdk_include_dirs("esp32s3", None); + assert!( + dirs.contains(&scanned), + "duplicate-padded script should have been rejected in favour of the tree scan" + ); +}