Skip to content

perf(memtrack): bound ring reads and resume paused pids at low fill - #549

Open
not-matthias wants to merge 1 commit into
feat/memtrack-pause-workerfrom
cod-3659-bound-stacks-ring-reads-and-release-paused-pids-at-a-low
Open

not-matthias wants to merge 1 commit into
feat/memtrack-pause-workerfrom
cod-3659-bound-stacks-ring-reads-and-release-paused-pids-at-a-low

Conversation

@not-matthias

@not-matthias not-matthias commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Bound stacks-ring reads and resume pressure-stopped processes at low fill.

A regular poll tick drained the whole ring in one poll(Duration::ZERO), and pressure-stopped processes were only resumed after that tick ended with the ring completely empty. Measured on a large memory benchmark suite with stack capture (CODSPEED_MEMTRACK_STATS, #546):

  • Single stacks-ring ticks ran 0.4–1.7 s and consumed up to ~3.6× the 512 MiB ring.
  • All 147 pressure stops happened inside such a tick.
  • Stop → resume took 238–513 ms (median per shard), so tracked processes were SIGSTOPped for 5–7% of the run.

Changes:

  • Regular ticks read in chunks of 1024 records with RingBuffer::consume_raw_n and check the fill between chunks.
  • Pressure-stopped processes resume once the ring is below 1/4 full (on_drained → on_low_fill) instead of at empty. BPF stops at 3/4, so the two thresholds leave room between stop and resume.
  • drain() still reads the ring fully, and shutdown still releases unconditionally.

Review note: release_pressure now also runs between chunks while the ring is below the watermark, which is one pressure_stopped key iteration per 1024 records on a busy ring.

pressure_tests.rs (BPF, root) is not in the CI bpf-tests matrix, so I ran it by hand on ubuntu-latest and ubuntu-24.04-arm, on this branch and on its base. I used ITERATIONS = 50000: at the committed 400000, the slow-poller phase was canceled on hosted runners both on this branch and on the base, most likely memory (the test reads the event channel only after the fixture exits). Both tests pass on both branches with dropped 0. Wall time of the paused runs (x86 / arm):

test base this PR
slow_poller_pause_recovers_without_loss (slow poller) 170.8 s / 162.7 s 23.4 s / 13.0 s
slow_poller_pause_resumes_every_writing_process 354.8 s / 336.1 s 40.5 s / 17.0 s

With the test's 10 s poll, base resumes paused processes once per tick (about every 10.6 s); this branch resumes them between chunks (about every 0.9 s).

Closes COD-3659

@codspeed

codspeed Bot commented Sep 25, 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-3659-bound-stacks-ring-reads-and-release-paused-pids-at-a-low (96b0517) with feat/memtrack-pause-worker (3d58b73)

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. ↩

@not-matthias
not-matthias marked this pull request as ready for review September 25, 2026 15:43
@greptile-apps

greptile-apps Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Changes when memory tracking resumes paused processes.

The PR should not merge until regular ticks can service drain requests under sustained ring production.

Fix All in Claude CodeFindings

  1. P1 Full chunks delay drain requests ▶
Fix with agent prompt
### Issue 1
crates/memtrack/src/ebpf/poller.rs:145-149
If producers keep supplying at least 1,024 records per read, this loop keeps consuming without checking the control channel. The attach worker can request a synchronous `drain()` while processes are still running, but its request cannot be acknowledged until the loop ends, so it can block indefinitely. Limit the work per tick or check for control requests between chunks.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR changes regular ring reads to 1,024-record chunks and resumes pressure-stopped stack producers below one-quarter ring fill.

  • Full draining on explicit drain and shutdown remains in place.
  • Repeated full chunks can still keep a tick from servicing a drain request.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Regular tick] --> B[Consume up to 1024 records]
  B --> C{Full chunk?}
  C -- Yes --> D[Check low fill]
  D --> B
  C -- No --> E[Return to control-channel receive]
  F[Drain request] --> E
Loading

Reviews (1) · Last reviewed commit: "fixup! perf(memtrack): bound ring reads ..."

Comment on lines +145 to +149
while ringbuf.consume_raw_n(CONSUME_CHUNK_RECORDS)
== CONSUME_CHUNK_RECORDS as i32
{
release_if_low();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Full chunks delay drain requests If producers keep supplying at least 1,024 records per read, this loop keeps consuming without checking the control channel. The attach worker can request a synchronous drain() while processes are still running, but its request cannot be acknowledged until the loop ends, so it can block indefinitely. Limit the work per tick or check for control requests between chunks.

Knowledge Base Used: eBPF memory tracker

Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/memtrack/src/ebpf/poller.rs
Line: 145-149

Comment:
**Full chunks delay drain requests** If producers keep supplying at least 1,024 records per read, this loop keeps consuming without checking the control channel. The attach worker can request a synchronous `drain()` while processes are still running, but its request cannot be acknowledged until the loop ends, so it can block indefinitely. Limit the work per tick or check for control requests between chunks.

**Knowledge Base Used:** [eBPF memory tracker](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/codspeed/-/docs/ebpf-memory-tracker.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code Fix in Codex

@not-matthias
not-matthias force-pushed the cod-3659-bound-stacks-ring-reads-and-release-paused-pids-at-a-low branch from 415571b to 4c24a9a Compare September 25, 2026 15:53
A regular poll tick drained the whole ring in one `poll(ZERO)`, and
pressure-stopped processes were resumed only after that tick ended with
the ring completely empty. On a large memory benchmark suite with stack
capture, single ticks of the stacks ring ran 0.4-1.7 s, every pressure
stop landed inside one, and stopped processes waited 238-513 ms (median)
to resume, 5-7% of the run.

Ticks now read in chunks of 1024 records with `consume_raw_n` and check
the fill between chunks. Paused processes resume once the ring is below
a quarter full instead of empty; BPF stops them at three quarters, so the
two thresholds leave room between stop and resume. `drain()` still reads
the ring fully, and shutdown still releases unconditionally.

Closes COD-3659
@not-matthias
not-matthias added this pull request to stack #545 September 25, 2026 15:55
@not-matthias
not-matthias force-pushed the cod-3659-bound-stacks-ring-reads-and-release-paused-pids-at-a-low branch from 4c24a9a to 96b0517 Compare September 25, 2026 15:55
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.

1 participant