Skip to content

Attribute herder/overlay heap in the periodic memory report - #3938

Open
tomerweller wants to merge 1 commit into
mainfrom
do/issue-3845
Open

Attribute herder/overlay heap in the periodic memory report#3938
tomerweller wants to merge 1 commit into
mainfrom
do/issue-3845

Conversation

@tomerweller

Copy link
Copy Markdown
Collaborator

Closes #3845

Summary

Adds a Weak-reporter registry to LedgerManager so the existing every-64-ledger memory report folds in per-subsystem heap components from the herder (tx queue, SCP-slot retention, fetching envelopes, quorum + externalize-lag maps) and overlay (flood gate, peer maps, known/banned peer sets). This directly targets the 97.5% unaccounted region the 42h soak in #3845 documented — the named components previously tracked only ~2.5% of a 10 GB anon swing.

The aggregation point stays where it is (the ledger-close build_memory_report), so new components are co-sampled with RSS/jemalloc exactly like today's; only what that call can see is widened. MemoryReporter lives in henyey-common, so the ledger holds Weak<dyn MemoryReporter> without naming concrete subsystem types and without a crate-dependency inversion. Weak is mandatory: the herder/overlay hold Arc<LedgerManager>, so a strong back-ref would form a leak-cycle. Dead reporters upgrade to None and are pruned.

Each memory_components() calls O(1)/O(bounded) estimate_heap_bytes() methods that read capacities/lengths only, take short-lived locks, and never re-enter the ledger — so the report path cannot deadlock. The weak-upgrade/merge/prune step is an extracted free helper (collect_and_prune_reporters) that releases the registry lock before invoking any reporter callback.

Plan reference

Converged Plan comment

Test plan

  • cargo fmt --check
  • cargo clippy -p henyey-common -p henyey-ledger -p henyey-herder -p henyey-overlay -p henyey-app --all-targets -- -D warnings
  • cargo test -p henyey-common -p henyey-ledger -p henyey-herder -p henyey-overlay — all green

New coverage (per the plan's "New coverage" list):

  • henyey-common: test_btreemap_heap_bytes_*, test_vecdeque_heap_bytes.
  • ledger registry: test_register_memory_reporter_appends_components, test_dropped_memory_reporter_is_skipped (via the extracted helper), plus test_register_memory_reporter_via_manager.
  • herder: one estimate_heap_bytes test per subsystem (tx_queue, fetching_envelopes, quorum_tracker, externalize_lag) + test_herder_memory_components_names.
  • overlay: flood-gate estimate monotonic-under-inserts + test_overlay_memory_components_names.

Parity considerations

n/a — no observable-surface change. Adds read-only heap estimates and additive heap_components_mb / "Memory report component" log fields only; estimate_heap_bytes() is pure. The reserved memory_report=true field is preserved (the test_memory_report_emits_field_* suite still passes).

Deviations from plan

  • Diff is ~710 lines (over the ~500-line soft threshold Critic C flagged), but it stays entirely within the plan's named files and the plan's main body required overlay + app wiring in this same PR so the trait mechanism ships with its full consumer set. Kept as one PR rather than taking the optional overlay split.

🤖 Generated with Claude Code

Add a Weak-reporter registry to LedgerManager so the every-64-ledger
memory report folds in per-subsystem heap components from the herder
(tx queue, SCP-slot retention, fetching envelopes, quorum + externalize
-lag maps) and overlay (flood gate, peer maps, known/banned peer sets),
shrinking the 97.5% unaccounted region the 42h soak documented.

- henyey-common: object-safe MemoryReporter trait + btreemap/vecdeque
  heap helpers.
- ledger: memory_reporters registry + register_memory_reporter (&self,
  Weak) + extracted collect_and_prune_reporters helper folded into
  build_memory_report after ledger-owned components (locks released
  first — no deadlock against reporter callbacks).
- herder/overlay: O(1) estimate_heap_bytes on the owned collections and
  MemoryReporter impls; app wires both registrations so the impls have a
  production call path.

Observability-only: read-only heap estimates and additive log fields;
no observable-surface change.

Refs #3845

Co-authored-by: Claude Code <claude-code@anthropic.com>
@tomerweller tomerweller added the pdr-managed PR opened by the henyey project-tick pipeline /do skill label Aug 27, 2026
@tomerweller

Copy link
Copy Markdown
Collaborator Author

🔍 Reviewer: Correctness

Verdict: APPROVE

Summary: Observability-only Weak-reporter registry with thorough unit coverage for every new public surface; no correctness concerns.

Full review

Cycle 1 (no prior ## 🔍 Reviewer: Correctness comment) — complete class-labeled assessment.

Issue kind: enhancement/feature. Test-verification gate (feature ⇒ every new pub surface has a test) is satisfied:

  • btreemap_heap_bytes / vecdeque_heap_bytes (common) — zero/monotonic/set-like/element-size tests.
  • estimate_heap_bytes on ExternalizeLagTracker, FetchingEnvelopes, SlotQuorumTracker, QuorumTracker, TransactionQueue, FloodGate, KnownPeerSet — each has a fresh-is-0 + grows-monotonically test.
  • MemoryReporter impls for Herder and OverlayManager — name-set assertion tests locking the exact component list.
  • collect_and_prune_reporters + register_memory_reporter — append, dead-weak prune, and via-manager build_memory_report tests.

Correctness:

  • Weak registry is the right call to avoid the herder/overlay Arc<LedgerManager> back-ref cycle; documented in the field docs. collect_and_prune_reporters snapshots live reporters and drops dead weaks under the registry lock, then releases it before invoking any memory_components() callback — no lock-order inversion against the ledger-close path, and prune never runs while a component guard is held. Correct and clearly reasoned.
  • TransactionQueue::estimate_heap_bytes takes store/seen/banned/account_states locks independently (never two at once), preserving the documented store → account_states → banned → seen order. No deadlock risk.
  • All estimators are O(1)/O(bounded) reads of capacities/lengths and never iterate large entry sets on the ledger-close path (FloodGate explicitly avoids the O(n) per-entry peers walk over ~1M entries). Conservative over/under-counts are documented per component.
  • btreemap_heap_bytes keys off len (no capacity() on BTreeMap) — the only available signal; monotonic and conservative.

No blocking concerns. Non-blocking notes (not gating): the ACCOUNT_KEY_HEAP_BYTES = 40 and IndexMap-as-hashmap approximations are coarse, but the trait contract explicitly permits conservative estimates, so this is acceptable for an observability feature.

@tomerweller

Copy link
Copy Markdown
Collaborator Author

🔍 Reviewer: Parity

Verdict: APPROVE

Summary: Purely internal observability — read-only heap estimates plus additive log fields in the periodic memory report; no observable/interop-surface change.

Full review

Cycle 1 (no prior ## 🔍 Reviewer: Parity comment) — complete class-labeled assessment.

The diff touches parity crates (herder, overlay, ledger, tx_queue), so parity lens applies. Per docs/PARITY.md, the observable/interop surface is: ledger/bucket hashes, tx result & meta XDR, SCP/overlay wire bytes, history archive format, HTTP/RPC/CLI contracts, crypto outputs.

This change touches none of them:

  • All new methods (estimate_heap_bytes, memory_components) are read-only — they read capacities/lengths/counters and mutate no state. No effect on SCP/overlay wire bytes, envelope handling, tx-queue admission, or ledger close.
  • The only externally-visible effect is additional named fields in the internal periodic memory report (logging/metrics) — explicitly in the "MAY deviate freely" category (metrics, logging, admin/debug endpoints).
  • No XDR, hash, crypto, or history-archive path is modified.
  • The Weak-reporter registry and registration wiring are internal architecture — allowed to differ from stellar-core.

No observable-surface concern. Nothing to flag under the parity lens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pdr-managed PR opened by the henyey project-tick pipeline /do skill

Projects

None yet

1 participant