Add MemAudit: per-subsystem heap accounting in the boot log - #10900
Conversation
The 2.8.0 nRF52840 heap-exhaustion field reports had to be diagnosed by hand, reconstructing each subsystem's heap footprint from source and build flags one report at a time. This makes every future report self-diagnosing from the serial log: a tiny fixed-size registry (src/memory/MemAudit.*) that big long-lived allocations report into, printed as one line at the end of setup() and alongside the periodic "Heap free:" log: MemAudit[boot]: tmm=2500 warm=4000 pkthist=5824 nodedb=13440 msgstore=2200 pktpool(live)=3270 total=31234 Instrumented: NodeDB hot vector (nodedb) + satellite maps (satmaps, rb-tree overhead estimated), WarmNodeStore (warm), PacketHistory records and hash index (pkthist), TrafficManagement caches (tmm/tmm_ni), MessageStore text pool (msgstore), TFT line/repaint buffers (display), and live in-flight packets (pktpool(live)) via an optional audit tag on the packet pool allocator - the one hot path, counted with a relaxed 32-bit atomic add (single instructions on Cortex-M, no locks). Cost: 128 B RAM for the 16-tag table, well under 1 KB flash on rak4631. MESHTASTIC_MEM_AUDIT=0 compiles it out to inline no-op stubs (call sites need no ifdefs); STM32WL, the tightest flash target, defaults off. New native suite test_mem_audit covers add/set/snapshot arithmetic, tag reuse (pointer and cross-TU strcmp fallback), null/unknown tags, and table-full behavior; test/native-suite-count bumped to 28.
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (26)
Build artifacts expire on 2026-08-05. Updated for |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a compile-time-gated memory audit registry ( ChangesMemory audit registry and instrumentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Subsystem as Subsystem code
participant MemAudit as memaudit registry
participant Logger as main.cpp / memGet.cpp
Subsystem->>MemAudit: add(tag, delta) / set(tag, bytes)
MemAudit->>MemAudit: register or reuse tag slot
MemAudit->>MemAudit: update byte counter
Logger->>MemAudit: logBreakdown(when)
MemAudit->>MemAudit: snapshot registered tags
MemAudit-->>Logger: log aggregated tag=bytes breakdown + total
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/memory/MemAudit.h (1)
7-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim the header comment block; exceeds guideline's comment-length limit.
This block spans ~19 lines of multi-paragraph explanation (design rationale, concurrency model, log format example). As per coding guidelines: "Keep code comments minimal: one or two lines at most, only explain the why when it is not obvious, and avoid multi-paragraph explanatory comments."
Consider trimming to a short summary and moving the detailed design rationale (concurrency semantics, tag-registration rules) to a design doc/PR description instead of inline comments.
🤖 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/memory/MemAudit.h` around lines 7 - 27, Trim the top-of-file comment in MemAudit.h to a minimal one- or two-line summary and remove the multi-paragraph design rationale, log example, concurrency details, and compilation notes. Keep only the essential purpose of MemAudit and any non-obvious constraint that callers must know, and move the deeper explanation about relaxed atomics, tag immutability, registration behavior, and no-op stubs into external documentation or the PR description.Source: Coding guidelines
src/memory/MemAudit.cpp (1)
93-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTag-length assumption isn't enforced at the write site.
The buffer sizing relies on tags never exceeding 16 characters (per the comment), but
add/set/findOrRegisternever validate tag length. A longer tag won't overflow the buffer (thepos + written >= sizeof(line)check catches truncation safely), but it will silently truncate the log line rather than fail loudly. Worth astatic_assert/length check at registration time ifMemAudit.hdoesn't already enforce this constraint.🤖 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/memory/MemAudit.cpp` around lines 93 - 112, The MemAudit breakdown logging assumes tags are at most 16 characters, but that constraint is not enforced where tags are registered. Add an explicit validation in the tag registration path used by add, set, and findOrRegister, or a static_assert if the limit is compile-time guaranteed, so oversized tags are rejected or flagged instead of silently truncating the output. Keep the check close to the tag creation logic in MemAudit and ensure the existing logBreakdown buffer sizing comment matches the enforced limit.src/modules/TrafficManagementModule.cpp (1)
207-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the "tmm_ni" clear the same way it's set.
memaudit::set("tmm_ni", 0)at Line 214 isn't wrapped in#if defined(ARCH_ESP32) && defined(BOARD_HAS_PSRAM), unlike the correspondingset("tmm_ni", ...)at Line 182. On platforms without PSRAM, "tmm_ni" is never actually populated but the destructor still registers it, permanently occupying a slot in the fixed-size tag table and showing a meaninglesstmm_ni=0in every breakdown log on those targets.♻️ Proposed fix
if (nodeInfoPayload) { if (nodeInfoPayloadFromPsram) free(nodeInfoPayload); else delete[] nodeInfoPayload; nodeInfoPayload = nullptr; +#if defined(ARCH_ESP32) && defined(BOARD_HAS_PSRAM) + memaudit::set("tmm_ni", 0); +#endif } - memaudit::set("tmm_ni", 0); }🤖 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/modules/TrafficManagementModule.cpp` around lines 207 - 215, The cleanup in TrafficManagementModule’s destructor clears the memaudit tag unconditionally, unlike the guarded set logic used earlier for tmm_ni. Update the teardown path around the nodeInfoPayload cleanup to wrap memaudit::set("tmm_ni", 0) with the same ARCH_ESP32 and BOARD_HAS_PSRAM preprocessor guard used when setting tmm_ni, so non-PSRAM builds do not register or retain that tag.src/mesh/NodeDB.cpp (1)
1801-1803: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim comment to comply with comment-length guideline.
This explanatory block is 3 lines; guidelines cap comments at one or two lines.
As per coding guidelines, "Keep code comments minimal: one or two lines at most, only explain the why when it is not obvious, and avoid multi-paragraph explanatory comments."
✏️ Suggested trim
- // Approximate satellite heap usage: each std::map entry is one rb-tree node, - // value_type plus ~44 B of node overhead (parent/left/right pointers, color, - // allocator rounding on 32-bit targets - an estimate, not exact bookkeeping). + // Approximate satellite heap usage (rb-tree node overhead ~44 B/entry, not exact). size_t satBytes = 0;🤖 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/mesh/NodeDB.cpp` around lines 1801 - 1803, Shorten the explanatory comment in NodeDB’s heap-usage note to fit the one- or two-line guideline; keep only the essential why/explanation and remove the extra detail about rb-tree internals, node overhead, and allocator rounding from the comment block near the heap usage estimate.Source: Coding guidelines
🤖 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 `@src/mesh/MemoryPool.h`:
- Line 153: Guard MemoryPool::release() against already-free slots before
changing audit state. In the release path, after the existing range/null checks
and before auditAdd on the freed slot, make sure the slot is still marked used
in the used[] tracking for MemoryPool<T>; if it is already free, return without
touching the audit counter. Update the logic around the assert(used[index])
check so release() does not decrement audit state in release builds for
double-frees.
---
Nitpick comments:
In `@src/memory/MemAudit.cpp`:
- Around line 93-112: The MemAudit breakdown logging assumes tags are at most 16
characters, but that constraint is not enforced where tags are registered. Add
an explicit validation in the tag registration path used by add, set, and
findOrRegister, or a static_assert if the limit is compile-time guaranteed, so
oversized tags are rejected or flagged instead of silently truncating the
output. Keep the check close to the tag creation logic in MemAudit and ensure
the existing logBreakdown buffer sizing comment matches the enforced limit.
In `@src/memory/MemAudit.h`:
- Around line 7-27: Trim the top-of-file comment in MemAudit.h to a minimal one-
or two-line summary and remove the multi-paragraph design rationale, log
example, concurrency details, and compilation notes. Keep only the essential
purpose of MemAudit and any non-obvious constraint that callers must know, and
move the deeper explanation about relaxed atomics, tag immutability,
registration behavior, and no-op stubs into external documentation or the PR
description.
In `@src/mesh/NodeDB.cpp`:
- Around line 1801-1803: Shorten the explanatory comment in NodeDB’s heap-usage
note to fit the one- or two-line guideline; keep only the essential
why/explanation and remove the extra detail about rb-tree internals, node
overhead, and allocator rounding from the comment block near the heap usage
estimate.
In `@src/modules/TrafficManagementModule.cpp`:
- Around line 207-215: The cleanup in TrafficManagementModule’s destructor
clears the memaudit tag unconditionally, unlike the guarded set logic used
earlier for tmm_ni. Update the teardown path around the nodeInfoPayload cleanup
to wrap memaudit::set("tmm_ni", 0) with the same ARCH_ESP32 and BOARD_HAS_PSRAM
preprocessor guard used when setting tmm_ni, so non-PSRAM builds do not register
or retain that tag.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec9a7b6f-12e5-4969-ba8b-7a73e2d76ed3
📒 Files selected for processing (14)
src/MessageStore.cppsrc/graphics/TFTDisplay.cppsrc/main.cppsrc/memGet.cppsrc/memory/MemAudit.cppsrc/memory/MemAudit.hsrc/mesh/MemoryPool.hsrc/mesh/NodeDB.cppsrc/mesh/PacketHistory.cppsrc/mesh/Router.cppsrc/mesh/WarmNodeStore.cppsrc/modules/TrafficManagementModule.cpptest/native-suite-counttest/test_mem_audit/test_main.cpp
| if (index >= 0 && index < MaxSize) { | ||
| assert(used[index]); // Should be marked as used | ||
| used[index] = false; | ||
| this->auditAdd(-(int32_t)sizeof(T)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '130,170p' src/mesh/MemoryPool.hRepository: meshtastic/firmware
Length of output: 1578
🏁 Script executed:
#!/bin/bash
sed -n '170,220p' src/mesh/MemoryPool.hRepository: meshtastic/firmware
Length of output: 389
Guard release() against already-free slots before adjusting audit state. alloc() already skips auditAdd() when the pool is full, but release() only checks range/null and then decrements after an assert(used[index]). In release builds, an already-free slot would still change the counter and can drift over time.
🤖 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/mesh/MemoryPool.h` at line 153, Guard MemoryPool::release() against
already-free slots before changing audit state. In the release path, after the
existing range/null checks and before auditAdd on the freed slot, make sure the
slot is still marked used in the used[] tracking for MemoryPool<T>; if it is
already free, return without touching the audit counter. Update the logic around
the assert(used[index]) check so release() does not decrement audit state in
release builds for double-frees.
The wasm env denies all sources and adds an explicit file list; MemAudit callers (main, MeshService, NodeDB, PacketHistory) are in that list but src/memory/MemAudit.cpp was not, so wasm-ld failed on undefined memaudit:: symbols.
Firmware Size Report22 targets | vs
Show 17 more target(s)
Updated for 5d3df00 |
|
On hardware (rak4631, 120-node NodeDB): the boot line renders exactly as designed and the arithmetic checks out against the ELF and live heap telemetry: It already earned its keep twice during validation: it exposed that the satellite maps ( |
…ic#10900) * Add MemAudit: per-subsystem heap accounting in the boot log The 2.8.0 nRF52840 heap-exhaustion field reports had to be diagnosed by hand, reconstructing each subsystem's heap footprint from source and build flags one report at a time. This makes every future report self-diagnosing from the serial log: a tiny fixed-size registry (src/memory/MemAudit.*) that big long-lived allocations report into, printed as one line at the end of setup() and alongside the periodic "Heap free:" log: MemAudit[boot]: tmm=2500 warm=4000 pkthist=5824 nodedb=13440 msgstore=2200 pktpool(live)=3270 total=31234 Instrumented: NodeDB hot vector (nodedb) + satellite maps (satmaps, rb-tree overhead estimated), WarmNodeStore (warm), PacketHistory records and hash index (pkthist), TrafficManagement caches (tmm/tmm_ni), MessageStore text pool (msgstore), TFT line/repaint buffers (display), and live in-flight packets (pktpool(live)) via an optional audit tag on the packet pool allocator - the one hot path, counted with a relaxed 32-bit atomic add (single instructions on Cortex-M, no locks). Cost: 128 B RAM for the 16-tag table, well under 1 KB flash on rak4631. MESHTASTIC_MEM_AUDIT=0 compiles it out to inline no-op stubs (call sites need no ifdefs); STM32WL, the tightest flash target, defaults off. New native suite test_mem_audit covers add/set/snapshot arithmetic, tag reuse (pointer and cross-TU strcmp fallback), null/unknown tags, and table-full behavior; test/native-suite-count bumped to 28. * native-wasm: add src/memory/ to the curated source filter The wasm env denies all sources and adds an explicit file list; MemAudit callers (main, MeshService, NodeDB, PacketHistory) are in that list but src/memory/MemAudit.cpp was not, so wasm-ld failed on undefined memaudit:: symbols.
1 participant
Motivation
The 2.8.0 nRF52840 field reports of ~99% heap use had to be diagnosed by hand: reconstructing which subsystem owned how much heap from source, build flags, and guesswork, one report at a time. This PR turns that community heap breakdown into a first-class feature — every future report self-diagnoses straight from the serial log.
What it does
A tiny fixed-size registry (
src/memory/MemAudit.h/.cpp, namespacememaudit) that subsystems report their large long-lived allocations into, printed as a single log line at the end ofsetup()and again alongside the existing periodicHeap free:line:add(tag, delta)/set(tag, bytes)/snapshot(out, max)/logBreakdown(when)strcmpfallback), up to 16 tags, no heap allocation in the registry itselfInstrumented subsystems
nodedbMAX_NUM_NODES * sizeof)NodeDB::nodeDBSelfCare()satmapsNodeDB::enforceSatelliteCaps()warmWarmNodeStorector/dtorpkthistPacketHistoryctortmm/tmm_niTrafficManagementModulector/dtormsgstoreMessageStorepool initdisplayTFTDisplaypktpool(live)meshtastic_MeshPackets (the only hot-path counter; atomic add)MemoryPool.hvia an optional allocator tag set inRouter.cppCost
logBreakdown192 B,findOrRegister132 B,set14 B,addinlined by LTO) plus the one-line call sites and tag literals — well under 1 KB totalMESHTASTIC_MEM_AUDIT=0compiles the whole thing out to inline no-op stubs (call sites need no#ifdefs). Default is on everywhere except STM32WL, which is already flash-tight and excluded by default.Verification
test_mem_audit(add/set/snapshot math, tag reuse incl. cross-TU duplicate literals, null/unknown tags, table-full behavior);test/native-suite-countbumped 27 -> 28542 test cases: 542 succeeded(28/28 suites,test_mem_auditPASSED)pio run -e rak4631: SUCCESSSummary by CodeRabbit