feat(stl): FL_NO_INLINE portable macro + cold-helper split for showPixels (#2773 item 2.1)#2830
Merged
Merged
Conversation
…xels (#2773 item 2.1) Two coupled changes for the binary-size meta tracker (#2773): ## 1. Portable `FL_NO_INLINE` macro + lint guard The codebase had 12+ bare `__attribute__((noinline))` uses scattered across `src/`. The bare GCC/Clang form silently no-ops under MSVC, so a host build through the Windows toolchain (CI's WASM/host tests) was quietly losing the `noinline` semantics — exactly the kind of latent regression #2773 item 2.1 needs to prevent. The hot/cold split that this PR introduces collapses straight back into the hot path if any future change strips the attribute on one toolchain. - `src/fl/stl/compiler_control.h`: new `FL_NO_INLINE` macro that expands to `__attribute__((noinline))` on GCC/Clang and `__declspec(noinline)` on MSVC. Pattern mirrors the existing `FL_NO_INLINE_IF_AVR` and `FL_CONSTRUCTOR` shims in the same file. - `ci/lint_cpp/bare_noinline_checker.py`: new lint check that bans `__attribute__((noinline))` and `__declspec(noinline)` in `src/` unless the file is `fl/stl/compiler_control.{h,cpp.hpp}` (the macro shim itself), under `third_party/`, or carries a `// ok noinline` suppression. Wired into `run_all_checkers.py` next to the existing `BareSnprintfChecker` (the same regression-guard pattern from #2773 item 1.5). - Migrated all 12 existing call sites to `FL_NO_INLINE`: - `src/fl/gfx/blur.cpp.hpp` (3 sites) - `src/fl/math/memmove.h` (2 sites — AVR-only memcpy8/memset8 prototypes) - `src/platforms/arm/stm32/fastspi_arm_stm32.h` (1 site) - `src/platforms/esp/32/core/fastspi_esp32_arduino.h` (1 site) - `src/platforms/esp/32/core/fastspi_esp32_idf.h` (1 site) - `src/platforms/esp/32/drivers/rmt/rmt_4/channel_driver_rmt4.h` (7 sites) - `src/platforms/esp/32/drivers/rmt/rmt_5/channel_driver_rmt.cpp.hpp` (2 sites) - `src/platforms/esp/8266/fastspi_esp8266.h` (1 site) - `src/platforms/shared/spi_bitbang/generic_software_spi.h` (1 site) - Added `#include "fl/stl/compiler_control.h"` to `src/fl/math/memmove.h` (the only migrated file that didn't already pull it in transitively). ## 2. `Channel::showPixels` cold-helper split (#2773 item 2.1) The meta tracker described item 2.1 as a `Channel::showPixels` "iterator scaling" tier. Disassembly of the current 3.8 KB function showed something different — the bulk of the size is per-frame **driver-resolution boilerplate**: - `fl::string busKey` constructor + literal initialization - `if (mDriverPreBound)` short-circuit AND the slow `selectDriverForChannel` path - One-shot `FL_ERROR` diagnostic with three formatted stream chains for the typed-Bus-miss hint (`fl::enableDrivers<>()` / `FastLED.enableAllDrivers()` / `addLeds<..., fl::Bus::X>()`) - One-shot `FL_ERROR` for the bus/chipset-mismatch case - One-shot `FL_ERROR` for the no-driver case On the stock Blink workload mDriverPreBound is `true` (legacy `addLeds<NEOPIXEL>` route via `ClocklessIdf5`'s constructor), so the entire dynamic-resolution + diagnostic block is dead-at-runtime but linked-into-binary. Extracting it into a `FL_NO_INLINE resolveDynamicDriver()` helper keeps the hot path compact and lets the compiler keep the cold body separately optimizable. ### Measured on ESP32-S3 Blink (PlatformIO + IDF 5.4) | Build | `firmware.bin` | |---|---:| | master (pre-this-PR) | 659,920 B | | this PR (default verbosity) | 659,776 B | | this PR + `-DFASTLED_LOG_VERBOSITY=0` | 616,144 B | Net default delta: **−144 B**. Modest vs the original ~3 KB projection — confirms the meta-issue's assumption that the slow path "dead-strips" on legacy builds was off (LTO+`-Og` doesn't lift the cold body out of the hot icache line on its own; the explicit hot/cold split gives the compiler the seam it needs to make conservative choices). The `-DFASTLED_LOG_VERBOSITY=0` path is unchanged at 616,144 B — verifying no regression on the opt-in slim build. The architectural cleanup is load-bearing for the next iterations: once `resolveDynamicDriver` is its own function, follow-up work can move the diagnostic strings entirely behind `FASTLED_LOG_VERBOSITY > 1` without touching the hot path, and the slow path itself can be extracted to a separate TU and gated for tree-shaking. ## Tests - [x] `bash compile esp32s3 --examples Blink --platformio` — succeeds - [x] `bash compile esp32s3 --examples Blink --platformio --defines FASTLED_LOG_VERBOSITY=0` — 616,144 B, identical to master at the same flag - [x] `uv run python ci/lint_cpp/bare_noinline_checker.py` — clean on src/ after the migration Refs: #2773 #2473 #2421 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
PR changed again? Review this PR in Change Stack to compare snapshots and stay oriented. 📝 WalkthroughWalkthroughThis PR standardizes noinline directives across FastLED by introducing a portable ChangesNoinline Standardization & Driver Resolution Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@ci/lint_cpp/bare_noinline_checker.py`:
- Around line 78-85: The block-comment handling currently skips entire lines
when toggling in_block_comment, which misses banned tokens that appear before a
"/*" or after a "*/"; update the logic around in_block_comment (the code that
checks for "/*" and "*/" and the variable in_block_comment) to scan and operate
on the parts of the line outside block-comment spans instead of continuing
immediately: when a "/*" is found, check the substring before it for banned
tokens, then set in_block_comment and continue scanning after any matching "*/"
(and when "*/" is found, check the substring after it for banned tokens and
clear in_block_comment); apply the same substring-aware approach to the other
occurrence of this logic later in the file (the block handling around the second
in_block_comment check) so tokens on the same line as comment delimiters are
still inspected.
- Line 54: The __init__ method is missing an explicit return type annotation;
update the constructor signature (the __init__ function in
bare_noinline_checker.py) to include an explicit None return type (i.e., change
def __init__(self): to def __init__(self) -> None:) to satisfy the project rule
that all function return types must be annotated.
In `@src/fl/channels/channel.cpp.hpp`:
- Around line 343-344: The comment currently references the wrong macro name
`FL_NOINLINE`; update the comment to use the correct macro `FL_NO_INLINE` (the
sentence describing why the helper is marked noinline that references
`showPixels` should read `FL_NO_INLINE`) so the documentation matches the actual
symbol used in the code.
In `@src/fl/stl/compiler_control.h`:
- Around line 333-341: The new comment block for the FL_NO_INLINE macro contains
non-ASCII punctuation (e.g., em-dashes and other Unicode punctuation); edit the
comment around the FL_NO_INLINE usage description in compiler_control.h to
replace all non-ASCII punctuation with plain ASCII equivalents (use
hyphen/minus/double-hyphen for dashes, straight quotes, plain ellipsis or three
dots, etc.) so the file is 7-bit ASCII compliant.
🪄 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: 16e63967-6707-4d06-97df-2a33cd7e885d
📒 Files selected for processing (14)
ci/lint_cpp/bare_noinline_checker.pyci/lint_cpp/run_all_checkers.pysrc/fl/channels/channel.cpp.hppsrc/fl/channels/channel.hsrc/fl/gfx/blur.cpp.hppsrc/fl/math/memmove.hsrc/fl/stl/compiler_control.hsrc/platforms/arm/stm32/fastspi_arm_stm32.hsrc/platforms/esp/32/core/fastspi_esp32_arduino.hsrc/platforms/esp/32/core/fastspi_esp32_idf.hsrc/platforms/esp/32/drivers/rmt/rmt_4/channel_driver_rmt4.hsrc/platforms/esp/32/drivers/rmt/rmt_5/channel_driver_rmt.cpp.hppsrc/platforms/esp/8266/fastspi_esp8266.hsrc/platforms/shared/spi_bitbang/generic_software_spi.h
| class BareNoInlineChecker(FileContentChecker): | ||
| """Bans bare `noinline` compiler attributes in src/ (#2773 item 2.1 follow-up).""" | ||
|
|
||
| def __init__(self): |
There was a problem hiding this comment.
Add explicit return annotation for __init__.
Use def __init__(self) -> None: to satisfy the Python annotation rule.
As per coding guidelines, "**/*.py: All function return types MUST have explicit type annotations."
🤖 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 `@ci/lint_cpp/bare_noinline_checker.py` at line 54, The __init__ method is
missing an explicit return type annotation; update the constructor signature
(the __init__ function in bare_noinline_checker.py) to include an explicit None
return type (i.e., change def __init__(self): to def __init__(self) -> None:) to
satisfy the project rule that all function return types must be annotated.
Comment on lines
+78
to
+85
| if in_block_comment: | ||
| if "*/" in line: | ||
| in_block_comment = False | ||
| continue | ||
| if "/*" in line and "*/" not in line: | ||
| in_block_comment = True | ||
| continue | ||
|
|
There was a problem hiding this comment.
Block-comment handling can miss real violations.
The current flow skips whole lines when block comments open/close, so banned tokens can slip through when they appear before /* or after */ on the same line. That creates false negatives in enforcement.
Proposed fix sketch
- if in_block_comment:
- if "*/" in line:
- in_block_comment = False
- continue
- if "/*" in line and "*/" not in line:
- in_block_comment = True
- continue
+ # Strip block comments while preserving code outside comments.
+ code_line = line
+ if in_block_comment:
+ end = code_line.find("*/")
+ if end == -1:
+ continue
+ in_block_comment = False
+ code_line = code_line[end + 2 :]
+
+ while "/*" in code_line:
+ start = code_line.find("/*")
+ end = code_line.find("*/", start + 2)
+ if end == -1:
+ code_line = code_line[:start]
+ in_block_comment = True
+ break
+ code_line = code_line[:start] + code_line[end + 2 :]
if stripped.startswith("//"):
continue
if _SUPPRESS in line:
continue
- code = line.split("//")[0]
+ code = code_line.split("//", 1)[0]
if _BANNED_PATTERN.search(code):Also applies to: 91-93
🤖 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 `@ci/lint_cpp/bare_noinline_checker.py` around lines 78 - 85, The block-comment
handling currently skips entire lines when toggling in_block_comment, which
misses banned tokens that appear before a "/*" or after a "*/"; update the logic
around in_block_comment (the code that checks for "/*" and "*/" and the variable
in_block_comment) to scan and operate on the parts of the line outside
block-comment spans instead of continuing immediately: when a "/*" is found,
check the substring before it for banned tokens, then set in_block_comment and
continue scanning after any matching "*/" (and when "*/" is found, check the
substring after it for banned tokens and clear in_block_comment); apply the same
substring-aware approach to the other occurrence of this logic later in the file
(the block handling around the second in_block_comment check) so tokens on the
same line as comment delimiters are still inspected.
Comment on lines
+343
to
+344
| /// Marked `noinline` (via `FL_NOINLINE`) so the compiler doesn't fold the | ||
| /// cold body back into `showPixels`. The whole helper is reachable only |
There was a problem hiding this comment.
Fix macro name typo in the comment (FL_NO_INLINE).
The comment says FL_NOINLINE, but the macro is FL_NO_INLINE. Please align the text to the actual symbol name.
🤖 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 `@src/fl/channels/channel.cpp.hpp` around lines 343 - 344, The comment
currently references the wrong macro name `FL_NOINLINE`; update the comment to
use the correct macro `FL_NO_INLINE` (the sentence describing why the helper is
marked noinline that references `showPixels` should read `FL_NO_INLINE`) so the
documentation matches the actual symbol used in the code.
Comment on lines
+333
to
+341
| // Unconditional "do not inline this function" attribute, portable across | ||
| // GCC/Clang/MSVC. Prefer this over a bare `__attribute__((noinline))` in | ||
| // `src/` — a lint check in `ci/lint_cpp/bare_noinline_checker.py` enforces | ||
| // the use of the macro so the GCC-only syntax doesn't sneak in. | ||
| // | ||
| // Typical use: cold helpers extracted out of a hot path so the compiler | ||
| // can't fold them back via inline expansion. Pair with the lint guard. | ||
| // | ||
| // Usage: FL_NO_INLINE void coldHelper() { ... } |
There was a problem hiding this comment.
Replace non-ASCII punctuation in the new comment block.
The added comments include Unicode punctuation (for example em dashes). Please convert these to plain ASCII to satisfy the repo's source encoding rule.
As per coding guidelines, "ASCII-only constraint: new/modified C++ and Python source must be 7-bit ASCII."
🤖 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 `@src/fl/stl/compiler_control.h` around lines 333 - 341, The new comment block
for the FL_NO_INLINE macro contains non-ASCII punctuation (e.g., em-dashes and
other Unicode punctuation); edit the comment around the FL_NO_INLINE usage
description in compiler_control.h to replace all non-ASCII punctuation with
plain ASCII equivalents (use hyphen/minus/double-hyphen for dashes, straight
quotes, plain ellipsis or three dots, etc.) so the file is 7-bit ASCII
compliant.
3 tasks
This was referenced Jun 5, 2026
zackees
added a commit
that referenced
this pull request
Jun 6, 2026
…NLINE cold helper (#2773 item 2.5 — easy share) Same cold-helper pattern that worked for #2773 items 2.1 (PR #2830) and 2.2 (PR #2838). Hot path of allocateTx (initial tryAllocateWords succeeds — the common Blink boot case where there's plenty of TX memory) was bloated by the single-buffer fallback retry + 4 FL_WARN diagnostic lines + 3 conditional suggestion blocks, all of which reach for fl::sstream operator<< chains. Extract them into handleAllocateTxFailure(), an FL_NO_INLINE member helper. Hot path now ends at: if (!tryAllocateWords(words_needed, true)) { return handleAllocateTxFailure(channel_id, mem_blocks, words_needed, networkActive); } Cold path is behaviorally unchanged. This captures the easy share of #2773 item 2.5's ~3-4 KB target. The full template-trait refactor (skip the runtime planner entirely when addLeds<> is called exactly once with a known length) is still TODO but is a multi-day effort with template-instantiation risk.
pull Bot
pushed a commit
to manmuqingshan/FastLED
that referenced
this pull request
Jun 6, 2026
…lper (FastLED#2773 item 2.2) Same pattern that worked for FastLED#2773 item 2.1 in PR FastLED#2830: the progressive memory-reduction retry loop inside ChannelEngineRMTImpl::createChannel is a cold path (only reached when the initial allocation fails — typically never, in the static addLeds<>-at-boot scenario), but its body contains FL_LOG_RMT/FL_WARN operator<< instantiations plus a per-iteration Rmt5ChannelConfig constructor that the linker has to keep live inside the hot function body. Move the retry block into attemptAllocationRecovery(), an FL_NO_INLINE member helper. The hot path (initial allocation succeeds) shrinks proportionally; the cold path is unchanged behaviorally. Also fix a pre-existing C++17 nested-namespace error in src/fl/net/network_detector.h that broke local quick-mode test builds under -Wc++17-extensions. (CI uses C++17+ so it didn't catch it.) Target: ~1-2 KB Blink savings on ESP32-S3.
zackees
added a commit
that referenced
this pull request
Jun 6, 2026
… into FL_NO_INLINE helper (#2773 follow-up to #2832) The two-branch `FL_ERROR(... << ...)` chain in `Channel::showPixels` that fires when a bound driver has been runtime-disabled (typically by `FastLED.setExclusiveDriver<OtherBus>()`) was still inlined into the hot path. The branch is one-shot (`mDisabledDriverWarned`) and unreachable in the steady state, so the ~10 `operator<<` instantiations were pure cold-path bloat sitting in showPixels' prologue/epilogue. Move into an anonymous-namespace helper marked `FL_NO_INLINE` (per the `bare_noinline_checker` introduced in #2830, not bare `__attribute__((noinline))`), matching the shape the maintainer adopted for `resolveDynamicDriver()` in PR #2830. This is the leftover piece from PR #2832, which the maintainer flagged in the closing comment as worth a follow-up: > The two pieces this PR has that #2830 didn't (emitDisabledDriverError > and the #if FASTLED_HAS_DBG gate on ChannelManager::addDriver's > capStr builder) are small enough (~couple hundred bytes) that a > rebase round trip isn't worth the churn vs just reopening them in > a follow-up. The `addDriver` capStr DBG gate piece of #2832 was independently shipped by the maintainer as iter 6 (commit `ea96b373b5`, `perf(channels): gate addDriver capStr builder behind FASTLED_HAS_DBG (#2773 iter 6)`), so this rebase drops that half and keeps only the unique `emitDisabledDriverError` extraction. Refs #2773. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 6, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two coupled changes from #2773 item 2.1's punch list:
FL_NO_INLINEmacro + lint guard — sweep 12 bare__attribute__((noinline))uses insrc/to the portable macro, add a lint check (ci/lint_cpp/bare_noinline_checker.py) that prevents future regressions.Channel::showPixelscold-helper split — extractresolveDynamicDriver()so the hot legacyaddLeds<>path stays compact.User feedback that prompted #1: "we should FL_NO_INLINE, and add a lint for
__attribute__((noinline))so we don't have this again".Why the macro
The bare
__attribute__((noinline))form silently no-ops under MSVC, so a host build through the Windows toolchain (CI's WASM/host tests) loses thenoinlinesemantics — exactly the kind of latent regression #2773 item 2.1 needs to prevent. The hot/cold split this PR introduces collapses back into the hot path if any future change strips the attribute on one toolchain.Why the showPixels split
The meta tracker described 2.1 as an "iterator scaling" issue. Disassembling the current 3.8 KB
Channel::showPixelsshowed the bulk is per-frame driver-resolution boilerplate, not iterator code:fl::string busKeyconstruction, slowselectDriverForChannelpath, three formattedFL_ERRORstream chains for the typed-Bus-miss hints, etc.On stock Blink (legacy
addLeds<NEOPIXEL>→ClocklessIdf5constructor pre-binds the driver),mDriverPreBound == trueand the entire dynamic-resolution + diagnostic block is dead-at-runtime but linked. Extracting it into aFL_NO_INLINE resolveDynamicDriver()helper gives the compiler the seam it needs to keep the hot path compact.Measured on ESP32-S3 Blink (PlatformIO + IDF 5.4)
firmware.bin-DFASTLED_LOG_VERBOSITY=0Net default delta: −144 B. Much smaller than the original ~3 KB projection — confirms the meta-issue's assumption that the slow path "dead-strips" on legacy builds was off (LTO +
-Ogdoesn't lift the cold body on its own; the explicit hot/cold split gives the compiler the seam it needs). The-DFASTLED_LOG_VERBOSITY=0path is unchanged — verifying no regression on the opt-in slim build.The architectural cleanup is load-bearing for the next iterations: once
resolveDynamicDriveris its own function, follow-up work can extract the diagnostic strings entirely behind a verbosity gate without touching the hot path, and the slow path can be moved to a separate TU and gated for tree-shaking.Lint guard (item 2.1 follow-up)
ci/lint_cpp/bare_noinline_checker.py:__attribute__((noinline))and__declspec(noinline)insrc/fl/stl/compiler_control.{h,cpp.hpp})src/third_party/(upstream code we don't control)// ok noinlineat end of lineMigrated all 12 existing call sites:
src/fl/gfx/blur.cpp.hppsrc/fl/math/memmove.hmemcpy8/memset8)src/platforms/arm/stm32/fastspi_arm_stm32.hsrc/platforms/esp/32/core/fastspi_esp32_arduino.hsrc/platforms/esp/32/core/fastspi_esp32_idf.hsrc/platforms/esp/32/drivers/rmt/rmt_4/channel_driver_rmt4.hsrc/platforms/esp/32/drivers/rmt/rmt_5/channel_driver_rmt.cpp.hppsrc/platforms/esp/8266/fastspi_esp8266.hsrc/platforms/shared/spi_bitbang/generic_software_spi.hPlus
#include \"fl/stl/compiler_control.h\"added tosrc/fl/math/memmove.h(the only file that wasn't pulling it in transitively).Tests
bash compile esp32s3 --examples Blink --platformio— succeeds, 659,776 Bbash compile esp32s3 --examples Blink --platformio --defines FASTLED_LOG_VERBOSITY=0— 616,144 B (identical to master at the same flag)uv run python ci/lint_cpp/bare_noinline_checker.py— clean on src/ after the migrationRefs: #2773 #2473 #2421
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
FL_NO_INLINEmacro for better cross-platform compatibility.Chores
FL_NO_INLINEmacro instead of bare compiler-specific directives.