Skip to content

perf(memtrack): stop capturing stacks on free - #558

Open
not-matthias wants to merge 1 commit into
mainfrom
cod-3703-eval-performance-overhead-of-memory-flamegraphs
Open

not-matthias wants to merge 1 commit into
mainfrom
cod-3703-eval-performance-overhead-of-memory-flamegraphs

Conversation

@not-matthias

Copy link
Copy Markdown
Member

Why

Memory flamegraphs only attribute allocations. The platform's memtrack-parser unwinds stacks attached to Free events and caches the callchain, but only allocation samples are folded, so free stacks are never read.

With stack capture on by default, every free() still paid the full capture: an 8 KiB user-stack copy, the FNV hash, bpf_get_stackid, and (since exact-hash dedup is ~0% effective) a full record into the stack ring. That is roughly half of all captures.

What

  • free uprobe no longer calls capture_stack; submit_free_event drops the hash.
  • MemtrackEventKind::Free is a unit variant. The serialized form is unchanged (stack_hash was already skipped when zero), and artifacts written by older memtrack versions with a stack_hash on frees still decode (covered by legacy_free_with_stack_hash_decodes).
  • Stack test snapshots print Free without the has_stack flag.

Expected effect

About half the stack captures, stack ring writes, encode CPU, and archive stack bytes on allocation-heavy benchmarks; less unwinding work in callgraph generation. To be measured on the platform memory shards (COD-3703).

@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Memory tracking stops capturing call stacks on free operations.

The PR appears safe to merge, with a non-blocking gap in legacy artifact regression coverage.

Summary

The PR removes stack capture from free() while retaining a defaulted Free hash field in the shared artifact model. It updates parsing and snapshots to represent frees without captured stacks.

Reviews (2) · Last reviewed commit: "perf(memtrack): stop capturing stacks on..."

Memory flamegraphs only attribute allocations: the platform unwinds free
stacks and never reads the result. Capturing them doubled the per-event
probe cost, the stack ring traffic, and the archive's stack bytes.

The Free event no longer carries a stack_hash. Artifacts from older memtrack
versions that still have the field decode unchanged.

Refs COD-3703
@not-matthias
not-matthias force-pushed the cod-3703-eval-performance-overhead-of-memory-flamegraphs branch from 96e3091 to 6a35299 Compare September 30, 2026 14:49
@codspeed

codspeed Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 33 untouched benchmarks
⏩ 4 skipped benchmarks1


Comparing cod-3703-eval-performance-overhead-of-memory-flamegraphs (6a35299) with main (080ed5f)

Open in CodSpeed

Footnotes

  1. 4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@greptile-apps

greptile-apps Bot commented Sep 30, 2026

Copy link
Copy Markdown

Comments Outside Diff

These findings could not be posted inline.

  • P2 Legacy decode test removed crates/runner-shared/src/artifacts/memtrack/mod.rs:250 ▶

    The dedicated test for decoding an older Free event with a nonzero stack_hash was removed. This replacement checks serialization of values built with the current type, but never decodes an independently encoded legacy event. The code still promises that older artifacts decode, so a future regression could go unnoticed and silently end a streamed artifact at that event. Please keep a legacy-decode test with the new expected value.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@GuillaumeLagrange GuillaumeLagrange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

olgtm

@@ -60,6 +60,9 @@ pub enum MemtrackEventKind {
stack_hash: u64,
},
Free {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we release this already? Else outright delete it IMO, if it's just dev traces. WDYT? Else keep it like this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No release yet. So good point. I'll drop it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants