feat: replace PROC_INFO HashMap with EXEC_MAPPINGS LPM trie#80
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces per-process Changes
Sequence DiagramsequenceDiagram
participant Manager as DwarfUnwindManager
participant Refresh as DWARF Refresh
participant BPF as eBPF (EXEC_MAPPINGS LPM)
participant Unwind as DWARF Unwinder
Manager->>Manager: scan_and_update(tgid) -> Vec<ExecMapping>
Manager->>Refresh: emit MappingsDiff / mapping_diffs
Refresh->>BPF: send mapping_diffs -> update exec_mappings & dwarf_tgids
BPF->>BPF: for each ExecMapping -> summarize_address_range() -> insert LpmKey(tgid, addr_block)
Unwind->>BPF: lookup EXEC_MAPPINGS LpmKey(tgid, current_ip)
BPF-->>Unwind: ExecMapping (hit) / miss
alt mapping found
Unwind->>Unwind: use DWARF tables (shard_id, load_bias)
else mapping miss
Unwind->>Unwind: fallback to frame-pointer unwinding
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
profile-bee/src/dwarf_unwind.rs (1)
521-741:⚠️ Potential issue | 🟠 MajorRefresh can retain stale mappings when ranges are reused
The refresh path clones existing mappings and skips any mapping with the same
(begin, end). If a library isdlclosed and another is mapped at the same range (or a PID is reused), you keep the oldshard_id/load_bias/table_count, so LPM lookups return stale unwind data. Consider rebuilding the mappings list from scratch each scan (caches still prevent expensive re-parsing) or replacing entries when ranges match but metadata differs.🔧 Rebuild mappings each scan to avoid stale entries
- let existing = self.proc_mappings.get(&tgid); - let existing_ranges: Vec<(u64, u64)> = existing - .map(|mappings| mappings.iter().map(|m| (m.begin, m.end)).collect()) - .unwrap_or_default(); - - let mut mappings: Vec<ExecMapping> = existing.cloned().unwrap_or_default(); + let mut mappings: Vec<ExecMapping> = Vec::new(); ... - // Skip mappings we already have - if existing_ranges - .iter() - .any(|&(b, e)| b == start_addr && e == end_addr) - { - continue; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/src/dwarf_unwind.rs` around lines 521 - 741, The current refresh clones existing mappings (existing / existing_ranges / mappings) and skips any map with the same (begin,end), which can retain stale shard_id/load_bias/table_count when ranges get reused; instead rebuild the mappings list from scratch each scan: stop cloning existing into mappings and remove the early skip that checks existing_ranges.iter().any(|&(b,e)| b==start_addr && e==end_addr), always compute load_bias/shard_id/table_count for each executable mapping (caches like metadata_cache/binary_cache/path_cache still used to avoid expensive parsing), then push into mappings and finally self.proc_mappings.insert(tgid, mappings) as before.
🧹 Nitpick comments (1)
profile-bee/bin/profile-bee.rs (1)
1150-1150: Consider avoiding full proc_mappings clones on every refresh
send_refreshclones all mappings on each update. With many tracked PIDs, this can become a noticeable allocation and CPU hotspot (the refresh loop runs every ~1s). Consider sending only the updated TGIDs/mappings or a diff set to reduce cloning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/bin/profile-bee.rs` at line 1150, The current send_refresh path clones the entire manager.proc_mappings into proc_mappings (manager.proc_mappings.iter().map(|(&t, m)| (t, m.clone())).collect()), which is expensive; instead, modify send_refresh to only clone and send changed TGIDs/mappings by tracking updates (e.g., add a dirty set or per-TGID version in the manager) and iterate that set to collect Vec<(u32, Vec<ExecMapping>)> of deltas, or switch manager.proc_mappings values to Arc<Vec<ExecMapping>>/Rc so only cheap reference clones are done; update the code that consumes proc_mappings to accept diffs or Arced mappings and ensure symbol names referenced are manager.proc_mappings, proc_mappings, ExecMapping, TGID, and the send_refresh function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@profile-bee-common/src/lib.rs`:
- Around line 123-143: The ExecMappingKey struct is currently #[repr(C, packed)]
causing unaligned 64-bit access; change it to #[repr(C)] and add an explicit
4-byte padding field (e.g., pub _pad: u32) between tgid and address so address
is 8-byte aligned while preserving big-endian semantics and the existing derives
(Copy, Clone, Debug, Eq, PartialEq, Hash); update the struct declaration for
ExecMappingKey accordingly and then rebuild the eBPF and userspace artifacts.
In `@profile-bee/bin/profile-bee.rs`:
- Around line 1213-1240: Replace the silent discard of trie.insert results with
explicit error handling: instead of using "let _ = trie.insert(...)" in the loop
that iterates update.proc_mappings and
profile_bee::dwarf_unwind::summarize_address_range, match or if let Err(e) =
trie.insert(...) and emit a clear log (include tgid, mapping.begin/mapping.end,
block.addr and block.prefix_len, and the error) so map-full or other failures
are visible; keep inserting on success and decide whether to continue or abort
based on the returned error (preferably continue after logging to attempt other
entries).
In `@profile-bee/src/dwarf_unwind.rs`:
- Around line 79-105: The loop in summarize_address_range can overflow when
computing (high - curr + 1); change the range-length computation to use u128 and
a saturating subtraction: compute let range_len = (high as
u128).saturating_sub(curr as u128).saturating_add(1) and derive the bits from
range_len.leading_zeros() (adjusting the same -1/64 arithmetic as before), then
compute number_of_bits = std::cmp::min(curr.trailing_zeros(), /*bits derived
from range_len*/); keep curr as u64 and advance it with curr =
curr.wrapping_add(1u64 << number_of_bits) (or curr += 1 << number_of_bits) and
preserve the existing u64 wrap/stop check; this ensures no wrap when high is
near u64::MAX while keeping the rest of summarize_address_range logic intact.
---
Outside diff comments:
In `@profile-bee/src/dwarf_unwind.rs`:
- Around line 521-741: The current refresh clones existing mappings (existing /
existing_ranges / mappings) and skips any map with the same (begin,end), which
can retain stale shard_id/load_bias/table_count when ranges get reused; instead
rebuild the mappings list from scratch each scan: stop cloning existing into
mappings and remove the early skip that checks
existing_ranges.iter().any(|&(b,e)| b==start_addr && e==end_addr), always
compute load_bias/shard_id/table_count for each executable mapping (caches like
metadata_cache/binary_cache/path_cache still used to avoid expensive parsing),
then push into mappings and finally self.proc_mappings.insert(tgid, mappings) as
before.
---
Nitpick comments:
In `@profile-bee/bin/profile-bee.rs`:
- Line 1150: The current send_refresh path clones the entire
manager.proc_mappings into proc_mappings (manager.proc_mappings.iter().map(|(&t,
m)| (t, m.clone())).collect()), which is expensive; instead, modify send_refresh
to only clone and send changed TGIDs/mappings by tracking updates (e.g., add a
dirty set or per-TGID version in the manager) and iterate that set to collect
Vec<(u32, Vec<ExecMapping>)> of deltas, or switch manager.proc_mappings values
to Arc<Vec<ExecMapping>>/Rc so only cheap reference clones are done; update the
code that consumes proc_mappings to accept diffs or Arced mappings and ensure
symbol names referenced are manager.proc_mappings, proc_mappings, ExecMapping,
TGID, and the send_refresh function.
There was a problem hiding this comment.
Pull request overview
This pull request replaces the HashMap-based process info lookup (PROC_INFO) with an LPM (Longest Prefix Match) trie-based approach (EXEC_MAPPINGS) for executable mapping lookups during DWARF-based stack unwinding. This optimization improves the performance of address-to-mapping resolution from O(n) linear scan to O(log n) trie lookup, while removing the previous per-process limit of 8 executable mappings.
Changes:
- Replaced
PROC_INFOHashMap withEXEC_MAPPINGSLPM trie for O(log n) mapping lookups - Removed
MAX_PROC_MAPSlimitation, allowing processes to have unlimited executable mappings - Added
summarize_address_rangealgorithm to decompose memory ranges into LPM prefix blocks
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| profile-bee-common/src/lib.rs | Added ExecMappingKey structure and MAX_EXEC_MAPPING_ENTRIES constant for LPM trie |
| profile-bee-ebpf/src/lib.rs | Replaced PROC_INFO HashMap with EXEC_MAPPINGS LPM trie; updated lookup logic in unwinding functions |
| profile-bee/src/dwarf_unwind.rs | Changed proc_info to proc_mappings using Vec; added summarize_address_range function |
| profile-bee/src/ebpf.rs | Updated update_dwarf_tables to populate LPM trie instead of HashMap |
| profile-bee/bin/profile-bee.rs | Updated apply_dwarf_refresh to use LPM trie and handle Vec-based mappings |
| profile-bee/tests/dwarf_unit.rs | Updated tests to use proc_mappings instead of proc_info |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| res | ||
| } | ||
|
|
There was a problem hiding this comment.
The summarize_address_range function lacks unit tests to verify correctness for edge cases. Consider adding tests for:
- Empty ranges where
low > high(e.g., frommapping.end.saturating_sub(1)whenend == begin) - Maximum range
(0, u64::MAX)to verify overflow handling - Ranges that span power-of-2 boundaries
- Single-address ranges where
low == high
The overflow protection at line 102-104 appears correct but should be validated with tests.
Suggested change
| #[cfg(test)] | |
| mod tests { | |
| use super::{summarize_address_range, AddressBlockRange}; | |
| use std::collections::HashSet; | |
| /// Expand a set of AddressBlockRange entries into the set of covered addresses. | |
| /// This is only used for small test ranges to validate coverage and alignment. | |
| fn expand_blocks(blocks: &[AddressBlockRange]) -> HashSet<u64> { | |
| let mut addrs = HashSet::new(); | |
| for block in blocks { | |
| let prefix_len = block.prefix_len; | |
| assert!( | |
| prefix_len <= 64, | |
| "prefix_len must be in [0, 64], got {}", | |
| prefix_len | |
| ); | |
| let size: u128 = if prefix_len == 0 { | |
| 1u128 << 64 | |
| } else { | |
| 1u128 << (64 - prefix_len) | |
| }; | |
| let start = block.addr as u128; | |
| // Block must be aligned to its size (power-of-two boundary). | |
| assert_eq!( | |
| start % size, | |
| 0, | |
| "block at 0x{:x} with prefix {} is not aligned", | |
| block.addr, | |
| prefix_len | |
| ); | |
| let end = start + size - 1; | |
| // For tests we only call this with ranges small enough that this | |
| // enumeration is feasible. | |
| for a in start..=end { | |
| addrs.insert(a as u64); | |
| } | |
| } | |
| addrs | |
| } | |
| #[test] | |
| fn summarize_empty_range_returns_empty() { | |
| let blocks = summarize_address_range(10, 5); | |
| assert!( | |
| blocks.is_empty(), | |
| "expected empty result for low > high, got {:?}", | |
| blocks | |
| ); | |
| } | |
| #[test] | |
| fn summarize_full_address_space_single_block() { | |
| let blocks = summarize_address_range(0, u64::MAX); | |
| assert_eq!( | |
| blocks.len(), | |
| 1, | |
| "expected a single block summarizing full address space, got {:?}", | |
| blocks | |
| ); | |
| let b = &blocks[0]; | |
| assert_eq!(b.addr, 0, "full-space summary should start at 0"); | |
| assert_eq!( | |
| b.prefix_len, 0, | |
| "full-space summary should have prefix_len 0 (covers all 64 bits)" | |
| ); | |
| } | |
| #[test] | |
| fn summarize_single_address_range() { | |
| let addr = 0x1234_5678_9abc_def0u64; | |
| let blocks = summarize_address_range(addr, addr); | |
| assert_eq!( | |
| blocks.len(), | |
| 1, | |
| "expected a single block for single-address range, got {:?}", | |
| blocks | |
| ); | |
| let b = &blocks[0]; | |
| let size: u128 = if b.prefix_len == 0 { | |
| 1u128 << 64 | |
| } else { | |
| 1u128 << (64 - b.prefix_len) | |
| }; | |
| let start = b.addr as u128; | |
| let end = start + size - 1; | |
| // The block must cover the given address. | |
| assert!( | |
| (start as u64) <= addr && (end as u64) >= addr, | |
| "block [{:#x}, {:#x}] does not cover address {:#x}", | |
| start, | |
| end, | |
| addr | |
| ); | |
| // For a precise single-address range we expect the block to be of size 1 | |
| // and aligned at that exact address. | |
| assert_eq!(size, 1, "single-address range should produce block of size 1"); | |
| assert_eq!(b.addr, addr, "single-address block should start at the address"); | |
| } | |
| #[test] | |
| fn summarize_range_spanning_power_of_two_boundary() { | |
| // This range crosses the 0x1000 (4096) boundary: 0x0fff < 0x1000 <= 0x1100. | |
| let low = 0x0FFFu64; | |
| let high = 0x1100u64; | |
| let blocks = summarize_address_range(low, high); | |
| assert!( | |
| !blocks.is_empty(), | |
| "expected non-empty result for non-empty range" | |
| ); | |
| let covered = expand_blocks(&blocks); | |
| // All addresses in [low, high] must be covered. | |
| for a in low..=high { | |
| assert!( | |
| covered.contains(&a), | |
| "address {:#x} in input range is not covered by blocks {:?}", | |
| a, | |
| blocks | |
| ); | |
| } | |
| // And no addresses outside [low, high] may be covered. | |
| let expected_count = high - low + 1; | |
| assert_eq!( | |
| covered.len() as u64, | |
| expected_count, | |
| "blocks cover addresses outside the requested range" | |
| ); | |
| } | |
| } |
Comment on lines
+574
to
+577
| for block in crate::dwarf_unwind::summarize_address_range( | ||
| mapping.begin, | ||
| mapping.end.saturating_sub(1), | ||
| ) { |
There was a problem hiding this comment.
If a memory mapping has begin >= end (which should not happen with valid mappings from /proc/pid/maps but could occur with corrupted data), mapping.end.saturating_sub(1) would result in high < low being passed to summarize_address_range, returning an empty Vec. This would silently skip inserting LPM entries for that mapping without any error indication. Consider adding a debug assertion or validation check: debug_assert!(mapping.end > mapping.begin, "Invalid mapping range").
Comment on lines
+158
to
+163
| /// LPM trie for exec mapping lookups: maps (tgid, virtual_address) → ExecMapping. | ||
| /// Replaces the old proc_info HashMap + linear scan with O(log n) longest prefix match. | ||
| /// Userspace decomposes each memory mapping's address range into aligned power-of-2 | ||
| /// blocks and inserts each as a separate LPM trie entry. | ||
| #[map(name = "exec_mappings")] | ||
| pub static EXEC_MAPPINGS: LpmTrie<ExecMappingKey, ExecMapping> = |
There was a problem hiding this comment.
Documentation files reference the old PROC_INFO HashMap that has been replaced with EXEC_MAPPINGS LPM trie. The documentation should be updated to reflect the new architecture:
docs/dwarf_unwinding_design.mdline 14 and 30docs/tail_call_unwinding.mdline 119
Update these files to describe the LPM trie-based exec mapping lookup instead of the HashMap-based approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CHANGELOG.md (1)
7-13: Consider varying the bullet prefixes for readability. Several consecutive items start with “Fix…”, so a small rephrase would improve flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 7 - 13, The CHANGELOG entries are repetitive because many bullets start with "Fix…"; edit the listed bullets (references: PROC_INFO/EXEC_MAPPINGS, ExecMappingKey, summarize_address_range, send_refresh, trie.insert, and the LPM trie insert/logging entry) to vary the leading verbs for readability — e.g., replace some "Fix" with "Prevent", "Avoid", "Address", "Improve", or "Ensure" and rephrase one or two items to start with a noun or outcome (e.g., "Overflow protection in summarize_address_range" or "DWARF mapping refresh rebuilds process mappings") so the list reads more smoothly while preserving the exact technical details and symbols shown.profile-bee-ebpf/src/lib.rs (1)
626-634: Derive LPM key bit-length fromExecMappingKeysize to prevent silent breakage on future layout changes.The current hardcoded
128is correct (16 bytes × 8 = 128 bits), but a compile-time constant derived fromcore::mem::size_of::<ExecMappingKey>()eliminates the magic number. This aligns with the coding guideline requiring explicit validation of shared data structure layouts. Apply to all threeLpmKey::new(128, ...)call sites in this file:♻️ Proposed refactor
+const EXEC_MAPPING_KEY_BITS: u32 = + (core::mem::size_of::<ExecMappingKey>() * 8) as u32;Then replace each
LpmKey::new(128, ...)withLpmKey::new(EXEC_MAPPING_KEY_BITS, ...)across all occurrences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee-ebpf/src/lib.rs` around lines 626 - 634, The code uses a magic constant 128 when constructing LpmKey (LpmKey::new(128, ...)), which can break if ExecMappingKey layout changes; declare a compile-time constant like EXEC_MAPPING_KEY_BITS = core::mem::size_of::<ExecMappingKey>() * 8 and replace all three LpmKey::new(128, ...) call sites with LpmKey::new(EXEC_MAPPING_KEY_BITS, ...) so the bit-length is derived from ExecMappingKey's size rather than hardcoded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@profile-bee/bin/profile-bee.rs`:
- Around line 1079-1084: The code currently only calls send_refresh when
manager.refresh_process(new_tgid) returns non-empty new_shard_ids, which skips
DWARF refreshes for execs that changed mappings but not shard creation; remove
the !new_shard_ids.is_empty() guard so that after calling
manager.refresh_process(new_tgid) you always call send_refresh(&manager, &tx,
new_tgid, new_shard_ids) and handle its Result (return on Err) even when
new_shard_ids is empty; apply the same change to the other occurrence that calls
manager.refresh_process and send_refresh (the second block around send_refresh
usage).
---
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 7-13: The CHANGELOG entries are repetitive because many bullets
start with "Fix…"; edit the listed bullets (references: PROC_INFO/EXEC_MAPPINGS,
ExecMappingKey, summarize_address_range, send_refresh, trie.insert, and the LPM
trie insert/logging entry) to vary the leading verbs for readability — e.g.,
replace some "Fix" with "Prevent", "Avoid", "Address", "Improve", or "Ensure"
and rephrase one or two items to start with a noun or outcome (e.g., "Overflow
protection in summarize_address_range" or "DWARF mapping refresh rebuilds
process mappings") so the list reads more smoothly while preserving the exact
technical details and symbols shown.
In `@profile-bee-ebpf/src/lib.rs`:
- Around line 626-634: The code uses a magic constant 128 when constructing
LpmKey (LpmKey::new(128, ...)), which can break if ExecMappingKey layout
changes; declare a compile-time constant like EXEC_MAPPING_KEY_BITS =
core::mem::size_of::<ExecMappingKey>() * 8 and replace all three
LpmKey::new(128, ...) call sites with LpmKey::new(EXEC_MAPPING_KEY_BITS, ...) so
the bit-length is derived from ExecMappingKey's size rather than hardcoded.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
profile-bee/ebpf-bin/profile-bee.bpf.ois excluded by!**/*.o
📒 Files selected for processing (11)
CHANGELOG.mddocs/dwarf_unwinding_design.mddocs/tail_call_unwinding.mdprofile-bee-common/Cargo.tomlprofile-bee-common/src/lib.rsprofile-bee-ebpf/src/lib.rsprofile-bee-tui/Cargo.tomlprofile-bee/Cargo.tomlprofile-bee/bin/profile-bee.rsprofile-bee/src/dwarf_unwind.rsprofile-bee/src/ebpf.rs
✅ Files skipped from review due to trivial changes (1)
- profile-bee-common/Cargo.toml
Eliminates the MAX_PROC_MAPS=8 per-process mapping limit that caused silent data loss for processes with many shared libraries. The BPF verifier loop over mappings is completely removed, replaced by a single LPM trie lookup per frame. Key changes: - Add ExecMappingKey (big-endian tgid+address) to profile-bee-common - Replace PROC_INFO HashMap + linear scan with EXEC_MAPPINGS LpmTrie in BPF - Port summarize_address_range() from lightswitch for address decomposition - Update userspace to populate LPM trie with ~10-20 entries per mapping - Per-process limit goes from 8 to effectively unlimited (200K total entries) - Verification time unchanged (188.6ms -> 193.7ms, noise) - All 16 e2e tests and 51 unit tests pass
Fix ExecMappingKey alignment (#[repr(C)] with explicit padding instead of packed), overflow in summarize_address_range near u64::MAX, stale DWARF mappings on refresh, and silent trie.insert error discard. Optimize send_refresh to only clone changed process mappings. Add debug_assert for invalid mapping ranges, unit tests for address range edge cases, and update docs to reflect LPM trie architecture. Rebuild prebuilt eBPF binary with all changes.
zz85
force-pushed
the
lpm-trie-mappings
branch
from
da8745a to
745edd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dwarf_unwinding_design.md`:
- Around line 115-119: Update the doc to remove or replace references to
ProcInfo and MAX_PROC_MAPS and instead describe the new exec_mappings structure
and its limits; specifically, search for occurrences of "ProcInfo", "PROC_INFO",
and "MAX_PROC_MAPS" and replace the text with the new design notes for
exec_mappings (LPM Trie per-process exec mapping lookup by (tgid, address))
including updated entry counts, memory estimates and any sharding/eviction
behavior so the sections are consistent with the earlier table that lists
`exec_mappings` and `unwind_tables`.
In `@profile-bee/bin/profile-bee.rs`:
- Around line 1223-1263: apply_dwarf_refresh currently only inserts into the
exec_mappings LPM trie and never removes stale entries, which leaks entries when
processes exit; update apply_dwarf_refresh to prune stale mappings by comparing
current live TGIDs (from update.proc_mappings) against existing entries in the
LPM trie (retrieved via bpf.map_mut("exec_mappings") and
aya::maps::lpm_trie::LpmTrie) and removing entries whose ExecMappingKeyPod.tgid
is not in the live set (use trie.remove(&key) for matching keys), or
alternatively rebuild the trie each refresh by clearing the map and reinserting
only mappings from update.proc_mappings to ensure ExecMappingKeyPod entries
reflect current processes only.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
profile-bee/ebpf-bin/profile-bee.bpf.ois excluded by!**/*.o
📒 Files selected for processing (12)
CHANGELOG.mddocs/dwarf_unwinding_design.mddocs/tail_call_unwinding.mdprofile-bee-common/Cargo.tomlprofile-bee-common/src/lib.rsprofile-bee-ebpf/src/lib.rsprofile-bee-tui/Cargo.tomlprofile-bee/Cargo.tomlprofile-bee/bin/profile-bee.rsprofile-bee/src/dwarf_unwind.rsprofile-bee/src/ebpf.rsprofile-bee/tests/dwarf_unit.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- profile-bee-common/src/lib.rs
- profile-bee-common/Cargo.toml
…M trie refresh Add a dwarf_tgids BPF HashMap so the eBPF process-exit handler can detect when DWARF-tracked processes exit and notify userspace to clean up their LPM trie entries. Previously, trie entries leaked on exit. Key changes: - eBPF: new DWARF_TGIDS map, handle_process_exit checks both monitored PID and DWARF-tracked tgids - Userspace: MappingsDiff struct for incremental add/remove of LPM trie entries instead of re-sending all mappings on every refresh - DwarfThreadMsg enum replaces raw u32 channel to support both LoadProcess and ProcessExited messages - All three code paths (batch, streaming, TUI) forward exit events to the DWARF thread for cleanup, only stopping when the target PID exits - Update dwarf_unwinding_design.md for current data structures and maps
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@profile-bee/bin/profile-bee.rs`:
- Around line 1408-1418: On PerfWork::ProcessExit (and the two other similar
handlers at the other locations), after sending DwarfThreadMsg::ProcessExited
and before/after the monitor_exit_pid check, remove the exiting pid from the
in-memory set that tracks seen pids (known_tgids) so PID reuse won't block
future LoadProcess events; call the appropriate remove method (e.g.,
known_tgids.remove(&exit_event.pid) or via Mutex/lock if known_tgids is shared)
for the exact pid, keeping the existing DwarfThreadMsg send and monitor_exit_pid
logic intact.
- Around line 1316-1329: The current logic in the dwarf_tgids update loop
(bpf.map_mut("dwarf_tgids") / aya::maps::HashMap and iterating
update.mapping_diffs) removes a tgid whenever diff.removed is non-empty and
diff.added.is_empty(), which erroneously drops DWARF exit tracking on partial
mapping removals; change the removal predicate to only remove the tgid when the
diff explicitly indicates the process exited (e.g., an exit/terminated flag on
the diff structure such as diff.exited or diff.is_exit) and keep the entry for
partial unloads, so use diff.tgid removal only when that explicit exit marker is
true and otherwise do not call dwarf_tgids.remove.
In `@profile-bee/src/dwarf_unwind.rs`:
- Around line 760-784: The diff currently uses only (m.begin, m.end) as the
identity when computing old_set/new_set in the proc_mappings -> MappingsDiff
logic, which misses changes to mapping metadata (e.g., load_bias, shard_id,
table_count) and leaves stale data in the LPM trie; update the identity used in
old_set/new_set and the filters that build `removed` and `added` to include the
relevant ExecMapping metadata fields (for example, use a tuple like (m.begin,
m.end, m.load_bias, m.shard_id, m.table_count) or another unique key derived
from ExecMapping) so that `proc_mappings`, `MappingsDiff`, and the added/removed
detection correctly treat mappings with identical ranges but different metadata
as changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
profile-bee/ebpf-bin/profile-bee.bpf.ois excluded by!**/*.o
📒 Files selected for processing (5)
docs/dwarf_unwinding_design.mdprofile-bee-ebpf/src/lib.rsprofile-bee/bin/profile-bee.rsprofile-bee/src/dwarf_unwind.rsprofile-bee/src/ebpf.rs
…d diff identity Three correctness fixes for the process exit cleanup path: - Clear exiting PID from known_tgids in all three ProcessExit handlers (batch, streaming, TUI) so PID reuse triggers a fresh LoadProcess - Add is_exit flag to MappingsDiff; only remove tgid from dwarf_tgids BPF map on actual process exit, not on partial mapping changes like dlclose during periodic rescan - Include load_bias, shard_id, table_count in MappingsDiff identity so a dlclose+dlopen of a different library at the same address range is correctly detected as a remove+add rather than a no-op
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
profile-bee/bin/profile-bee.rs (1)
699-726:⚠️ Potential issue | 🟡 MinorNoisy
println!insetup_process_exit_ring_buffer_taskwill fire for every system-wide process exit when DWARF is enabled.Expanding the condition at Line 702 from
external_pid.is_some()toexternal_pid.is_some() || dwarf_enablednow startssetup_process_exit_ring_buffer_taskfor all DWARF sessions — including system-wide profiling with no--pid. The pre-existingprintln!("eBPF detected: PID {} has exited", exit_event.pid)at Line 1051 fires for every event delivered to the ring buffer, which on a busy host can be dozens per second.Consider downgrading Line 1051 to
tracing::debug!, or gating onmonitor_exit_pid:- println!("eBPF detected: PID {} has exited", exit_event.pid); + tracing::debug!("eBPF detected: PID {} has exited", exit_event.pid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/bin/profile-bee.rs` around lines 699 - 726, The noisy println in setup_process_exit_ring_buffer_task floods logs for every system-wide process exit when DWARF-only sessions are enabled; update the task to either change that println (e.g., "eBPF detected: PID {} has exited") to tracing::debug! or wrap the log behind a check of monitor_exit_pid/external_pid so it only logs when a specific PID is being monitored (use the existing monitor_exit_pid variable or test external_pid.is_some()); locate the logging inside setup_process_exit_ring_buffer_task and the related setup_process_exit_ring_buffer invocation and make the log conditional or lower-severity accordingly.
🧹 Nitpick comments (1)
profile-bee/src/dwarf_unwind.rs (1)
92-95:is_empty()ignoresis_exit— can skipdwarf_tgidscleanup in a degenerate case.
send_refreshshort-circuits withif shard_updates.is_empty() && diff.is_empty(). If a process exits whileproc_mappings[tgid]is already empty (all exec mappings had been removed before exit),diff.removedanddiff.addedare both empty,diff.is_empty()istrue, and the message is dropped — leaving the tgid indwarf_tgidsforever.In practice this is unreachable (a running process always retains its executable mapping), but making
is_empty()aware ofis_exitis safer:🛡️ Proposed fix
pub fn is_empty(&self) -> bool { - self.added.is_empty() && self.removed.is_empty() + self.added.is_empty() && self.removed.is_empty() && !self.is_exit }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@profile-bee/src/dwarf_unwind.rs` around lines 92 - 95, The current MappingsDiff::is_empty only checks added and removed and ignores is_exit, which lets send_refresh skip cleanup when a process exit happened; update the MappingsDiff::is_empty method to also consider the is_exit flag (i.e., return false if is_exit is true) so that diffs representing an exit are not treated as empty and will trigger dwarf_tgids cleanup in send_refresh; locate the is_empty implementation on struct MappingsDiff and modify its boolean expression to include !self.is_exit (or equivalent) alongside the existing added/removed checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@profile-bee/bin/profile-bee.rs`:
- Around line 699-726: The noisy println in setup_process_exit_ring_buffer_task
floods logs for every system-wide process exit when DWARF-only sessions are
enabled; update the task to either change that println (e.g., "eBPF detected:
PID {} has exited") to tracing::debug! or wrap the log behind a check of
monitor_exit_pid/external_pid so it only logs when a specific PID is being
monitored (use the existing monitor_exit_pid variable or test
external_pid.is_some()); locate the logging inside
setup_process_exit_ring_buffer_task and the related
setup_process_exit_ring_buffer invocation and make the log conditional or
lower-severity accordingly.
---
Nitpick comments:
In `@profile-bee/src/dwarf_unwind.rs`:
- Around line 92-95: The current MappingsDiff::is_empty only checks added and
removed and ignores is_exit, which lets send_refresh skip cleanup when a process
exit happened; update the MappingsDiff::is_empty method to also consider the
is_exit flag (i.e., return false if is_exit is true) so that diffs representing
an exit are not treated as empty and will trigger dwarf_tgids cleanup in
send_refresh; locate the is_empty implementation on struct MappingsDiff and
modify its boolean expression to include !self.is_exit (or equivalent) alongside
the existing added/removed checks.
…xit-aware - Change println! to tracing::debug! in setup_process_exit_ring_buffer_task to avoid log spam for every DWARF-tracked process exit - Include is_exit in MappingsDiff::is_empty() so exit-cleanup diffs are not skipped by the early return in send_refresh
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
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 by CodeRabbit