fix(linux): only continue past the stack copy through a matching snapshot - #11
Open
not-matthias wants to merge 5 commits into
Open
not-matthias wants to merge 5 commits into
not-matthias wants to merge 5 commits into
Conversation
not-matthias
force-pushed
the
cod-3660-investigate-cold-load-flamegraph-attribution-is-unreliable
branch
from
September 25, 2026 14:59
e9abbf7 to
63cc2c3
Compare
not-matthias
marked this pull request as ready for review
September 25, 2026 15:01
not-matthias
force-pushed
the
cod-3660-investigate-cold-load-flamegraph-attribution-is-unreliable
branch
from
September 25, 2026 15:04
63cc2c3 to
5c3afad
Compare
When a binary is found through the binary lookup dirs, the resolved path uses the host's separators. Splitting it at the last '/' kept the whole path as the library name on Windows. Path::file_name handles both.
tools/stack-cache-repro is a small program whose stacks are deeper than samply's 32000-byte user stack copy. Phase A recurses and works at every depth, then phase B recurses to the same depth through a different function and works only in its leaf, so phase B's frames above the copy sit where phase A's were seen earlier. replay.sh records it once with perf and imports the same perf.data with each given samply binary, so every build sees identical samples. stats.py counts stitched stacks (a phase B leaf under phase A's recursion, or the reverse) and exits non-zero if there are any. make-fixture.sh records a short run as a static binary into fixtures/other/stack-read-cache/, so a replay test can check the converter without perf permissions.
…shot Samples copy 32000 bytes of the user stack, starting at the sampled SP. The stack read cache answered unwinder reads above that copy with the word that some earlier sample of the process had seen at the same address. The thread has since returned and called other code at those addresses, so the unwinder followed stale return addresses and spliced an old call chain onto the current stack, e.g. RocksDB and serde frames under a pure in-memory recursive function in jazz profiles. Keep the last 64 captured windows per thread instead of a per-process address -> word map. A read above the current window is served by an earlier snapshot only if that snapshot starts inside the window and every word from its stable start (the lowest slot its own unwind read; below that are the leaf's live locals) up to the end of the window matches the current sample, over at least 512 words. Past that snapshot the same check chains to an older one, so each part of the stack comes from one earlier sample. Otherwise the stack is truncated. This is still a heuristic: a snapshot whose overlap is identical but whose callers above it differ is accepted. Tests: - unit tests on StackSnapshot::continues cover the 512-word threshold and that only words from the stable start upward must match; - tests/stack_read_cache_replay.rs imports the fixtures/other/stack-read-cache recording of tools/stack-cache-repro and checks that no stack is spliced across phases while phase A is still completed to its full depth. The old cache splices all 14 phase B stacks of this fixture.
The stack snapshots are keyed by tid, so a new thread that reuses an exited thread's tid (and possibly its stack mapping) could otherwise continue through the old thread's windows. Remove them when a non-main thread exits or execs; main-thread exit and exec already recreate the whole process.
Rust 1.98 adds useless_borrows_in_formatting and a format-width check, and making the samply modules public enabled the new_without_default, should_implement_trait and result_unit_err lints on their APIs.
not-matthias
force-pushed
the
cod-3660-investigate-cold-load-flamegraph-attribution-is-unreliable
branch
from
September 25, 2026 15:26
efad215 to
67471a7
Compare
Member
Author
|
Note: The test fixture is slop, I created it to validate that we can reproduce the issue + fix it; I've also validated on the upstream repo that the issue is fixed. So we can theoretically remove it, lmk what you think |
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.
Samples copy only 32 KB of the user stack. To unwind deeper, the stack read cache reused words that earlier samples had seen at the same addresses, without checking them. Once the thread had returned and called other code there, the unwinder followed stale return addresses and spliced an old call chain onto the current stack. In garden-co/jazz#3396 this put RocksDB frames under an in-memory recursive function.
Change
Validation
tests/stack_read_cache_replay.rsreplays a small committed perf.data (tools/stack-cache-repro). The old cache splices 70 of 70 phase B stacks; this one splices none.Limits