report building lock contention

[korniltsev-grafanista]

Where

src/report.rs:101-158 — build() delegates to build_and_clear(false), which unconditionally takes a write lock:

pub fn build(&self) -> Result {
self.build_and_clear(false) // src/report.rs:102
}

pub fn build_and_clear(&self, clear: bool) -> Result {
...
match self.profiler.write().as_mut() { // src/report.rs:118 ← write lock
...
profiler.data.try_iter()?.for_each(|entry| {
...
let mut key = Frames::from(entry.item.clone()); // symbolisation while holding the lock
...
});
if clear { profiler.clear()?; }
...
}
}

Contrast with build_unresolved() at src/report.rs:66, which correctly takes a read lock.

Why this is the worst bug in the repo

The SIGPROF handler at src/profiler.rs:324 uses PROFILER.try_write(). While any thread holds the write lock, every sample on every thread is silently dropped:

if let Some(mut guard) = PROFILER.try_write() { // src/profiler.rs:324
if let Ok(profiler) = guard.as_mut() {
...
}
}

Inside the write-locked section of build_and_clear, the expensive work happens:

• Frames::from(entry.item.clone()) at src/report.rs:127 resolves every frame's DWARF symbols via frame.resolve_symbol(...) (src/frames.rs:215), which hits debug info on disk.
• Every symbol name is then demangled via symbolic-demangle (src/frames.rs:128-130) during the flamegraph/pprof export pipeline.
• The whole thing is done for every unique frame in the HashMap.

Typical report builds on a real service take tens to hundreds of milliseconds. At 99 Hz × N threads that is a huge number of SIGPROFs arriving during that window, every single one of which fails try_write() and is thrown away. There is no counter, no log, no visibility — the samples just vanish.

Why it's especially bad for this particular fork

This is a regression introduced by the pyroscope patch itself (8c0f67e "feat: add Profiler::clear() and ReportBuilder::build_and_clear() for periodic profiling"). Upstream pprof-rs had build() under a read lock, so reports never blocked sampling. The fork refactored build() to route through build_and_clear(false) for code reuse, which unconditionally takes a write lock. So:

• Every existing caller of build() who upgrades to this fork silently loses samples during every report, even though they never asked for clearing.
• The pyroscope use case the fork was created for is continuous profiling with periodic reports. That is exactly the workload that hits this path repeatedly and loses the most
data.

The commit message PR #10 even advertises "periodic profiling" as the feature. The implementation defeats the goal.

The fix

Two levels, cheap to expensive:

1. Minimum fix — branch on clear: take profiler.read() when clear == false (matching build_unresolved), take profiler.write() only when actually clearing. That restores
   upstream behaviour for build() and makes the write lock pay only for the clearing path.

2. Real fix — do not symbolise under the lock at all: snapshot the raw UnresolvedFrames + count map under a short read lock (or drain under a short write lock if clearing),
   drop the lock, then run Frames::from(...) and the post-processor outside. This is what continuous profilers generally do. Total lock hold time drops from "hundreds of ms" to
   "a few ms", regardless of profile size.

Sketch of (1):

pub fn build_and_clear(&self, clear: bool) -> Result {
let mut hash_map = HashMap::new();
let collect = |profiler: &Profiler| -> Result<()> {
profiler.data.try_iter()?.for_each(|entry| { /* same body, no clear */ });
Ok(())
};
if clear {
match self.profiler.write().as_mut() {
Err() => return Err(Error::CreatingError),
Ok(profiler) => { collect(profiler)?; profiler.clear()?; }
}
} else {
match self.profiler.read().as_ref() {
Err() => return Err(Error::CreatingError),
Ok(profiler) => collect(profiler)?,
}
}
Ok(Report { data: hash_map, timing: self.timing.clone() })
}