Record tee hot-path profiling baseline (#127) - #171
Conversation
Run the full seven-scenario tee profiling matrix on commit `a50a631` (perf with DWARF call graphs, plus py-spy corroboration) against the seed-12345 1.5 GiB deterministic fixtures, and record the hotspot verdicts as the ADR-002 Phase 1 baseline evidence. Key verdicts: - capture double-touch (`bytearray.extend` plus the final `buffer.decode`) is ~51% of parent CPU in tee mode, supporting Phase 2 capture-only dispatch through `rust_consume_stream`; - per-line observe-hook dispatch dominates the line-callback path (~46x slowdown), driven by per-line `ExecEvent` construction, `inspect.isawaitable`, and `argv_with_program` recomputation — not by line splitting or incremental decode; - raising `_READ_SIZE` from 4 KiB to 16-64 KiB is a cheap ~20% win; - the Rust splice pump reduces multi-stage pumping cost from ~36 s to ~1 s, and the per-chunk flush and text-sink decode hypotheses are refuted. Also note two incidental findings: an FD-close race on the Rust pump path (`OSError: [Errno 9]` during transport shutdown) and a distro `perf script` quirk that drops Python JIT frames from the folded stacks while `perf report` resolves them.
|
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:
WalkthroughAdd documentation for the tee hot-path profiling baseline and a profiling-prerequisites subsection. Add an index entry linking to the new baseline document that records environment and fixture metadata, throughput metrics, six quantified hypothesis verdicts, incidental findings, ADR-002 phasing implications, and reproduction commands. Separately, update Kani testing guidance to compute toolchain paths dynamically. ChangesProfiling Baseline Documentation
Kani Testing Documentation Update
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds a new tee hot-path profiling baseline document as ADR-002 Phase 1 evidence and wires it into the docs index; no production code is changed. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Expand the ADR, PTY, FD, and JIT acronyms on first use per the documentation style guide. Derive the 84.77 M line-callback count from the wrap-76 fixture geometry, and document the exact inline driver used for the `_READ_SIZE` sweep so the reported gains are reproducible.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai Has this now been resolved in the latest commit? Use codegraph analysis to determine your answer. If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the current state of the profiling baseline document to check acronym expansions
cat docs/tee-hotpath-profiling-baseline-2026-06-12.mdLength of output: 11473 🏁 Script executed: #!/bin/bash
# List review comments by coderabbitai on PR `#171` to find comment IDs to resolve
gh api repos/leynos/cuprum/pulls/171/reviews \
--jq '.[] | select(.user.login == "coderabbitai") | {id: .id, state: .state, submitted_at: .submitted_at}'Length of output: 491 🏁 Script executed: #!/bin/bash
# Try the review comments (inline comments) endpoint
gh api repos/leynos/cuprum/pulls/171/comments \
--jq '.[] | select(.user.login == "coderabbitai") | {id: .id, path: .path, line: .line, body: (.body | .[0:80])}'Length of output: 471 🏁 Script executed: #!/bin/bash
# Try GraphQL to find review thread comments by coderabbitai
gh api graphql -f query='
{
repository(owner: "leynos", name: "cuprum") {
pullRequest(number: 171) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 5) {
nodes {
author { login }
body
databaseId
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.comments.nodes[0].author.login == "coderabbitai") | {threadId: .id, isResolved: .isResolved, body: (.comments.nodes[0].body | .[0:100])}'Length of output: 893 🏁 Script executed: #!/bin/bash
# Check all review threads on PR `#171`, regardless of author
gh api graphql -f query='
{
repository(owner: "leynos", name: "cuprum") {
pullRequest(number: 171) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 3) {
nodes {
author { login }
body
databaseId
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | {threadId: .id, isResolved: .isResolved, author: .comments.nodes[0].author.login, body: (.comments.nodes[0].body | .[0:120])}'Length of output: 1575
The two overall comments are also addressed: the 84.77 M line count is now derived explicitly in prose (2,175,740,011 bytes ÷ 77-byte lines × 3 repeats = 84,769,092 invocations), and the exact All four threads were already marked resolved on GitHub (by |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/contents.md`:
- Around line 34-35: The profiling baseline list item "[Tee hot-path profiling
baseline (2026-06-12)][tee-baseline]" currently sits under decision records but
should be moved into the "Planning and validation references" section; locate
the entry in docs/contents.md and cut/paste that list item into the "Planning
and validation references" (or similarly named validation references) subsection
so benchmark artefacts live with validation materials rather than the decision
index.
In `@docs/tee-hotpath-profiling-baseline-2026-06-12.md`:
- Around line 12-21: Add short Markdown captions under each table in the file:
the environment table (the block starting with the Git commit / CPU / Kernel
rows), the scenario results table(s), and the hypothesis-verdict table(s). For
each table insert a one-line caption immediately below the table using the
pattern from the docs style guide (e.g., "Table: Environment used for
profiling", "Table: Scenario results", "Table: Hypothesis and verdict"), so the
page passes the docs lint rule requiring captions; apply the same change to the
other table occurrences mentioned (the blocks around lines ~39-47 and ~132-138).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4b092fe8-dd83-487a-8809-54287767d968
📒 Files selected for processing (2)
docs/contents.mddocs/tee-hotpath-profiling-baseline-2026-06-12.md
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Move the tee profiling baseline index entry from "Design and decisions" to "Planning and validation references", since it is a benchmark artefact rather than a decision record. Add captions to all four tables in the baseline document per the documentation style guide rule requiring a caption on every table.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tee-hotpath-profiling-baseline-2026-06-12.md`:
- Around line 54-57: The prose describing wrap-76's line geometry is wrong:
update the sentence about the wrap-76 fixture so it explicitly states there are
28,256,363 full lines of 76 characters plus newline and a final partial line of
60 bytes (so each repeat emits 28,256,364 lines and three repeats yield
84,769,092 callbacks), replacing the current claim that all 28,256,364 lines are
full 76-char+newline lines; refer to the "wrap-76" fixture and the numeric
totals in the existing sentence and adjust wording and the split accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b45200e0-8689-4b8c-a76b-e72430cfcf16
📒 Files selected for processing (2)
docs/contents.mddocs/tee-hotpath-profiling-baseline-2026-06-12.md
Add a "Profiling prerequisites and build settings" subsection covering the tooling the baseline document depends on: frame-pointer Rust builds (`RUSTFLAGS="-C force-frame-pointers=yes"`), `PYTHONPERFSUPPORT=1`, the `perf record -F 999 -g --call-graph dwarf,16384` sampling defaults, and the required `perf`/`inferno`/`py-spy` tools. Link the baseline document and give the harness reproduction entrypoint.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 134-155: Update the prereq build instruction that currently reads
"RUSTFLAGS=\"-C force-frame-pointers=yes\" before `maturin develop --release`"
so it exactly mirrors the recorded command by including the crate manifest path
and the uv wrapper — i.e. ensure the step specifies `RUSTFLAGS="-C
force-frame-pointers=yes"` and then runs `uv run maturin develop --release
--manifest-path rust/cuprum-rust/Cargo.toml` (keep the `uv run` wrapper as shown
in the reproduction block) so readers running from the repo root use the correct
crate.
- Around line 130-143: Add a short prerequisite line that states the required
perf permission level for the documented record command: specify that the
baseline was captured with perf_event_paranoid=2 and show the sysctl command to
set it (sysctl -w kernel.perf_event_paranoid=2), or alternatively note that perf
must be run as root or the user granted CAP_SYS_ADMIN; place this immediately
after the "Install `perf`..." bullet and reference the recorded command `perf
record -F 999 -g --call-graph dwarf,16384` so readers know this prerequisite
applies to that sampling invocation and provide the equivalent sampling/config
option guidance for hardened hosts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 86003ba6-2770-4cde-adeb-e16311d517b4
📒 Files selected for processing (1)
docs/developers-guide.md
Record the `perf_event_paranoid` level the harness needs alongside the sampling command. The baseline ran at `perf_event_paranoid=2` (enough for user-space call graphs; kernel frames stay unresolved), with notes on checking the current level, lowering it for the session, or granting `CAP_PERFMON`.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Fix the wrap-76 fixture description: the 2,175,740,011 output bytes are 28,256,363 full 76-character lines plus a newline followed by a final 60-byte partial line with no trailing newline, not 28,256,364 full lines. Each repeat still emits 28,256,364 lines, so three repeats yield 84,769,092 callbacks. Align the developers' guide frame-pointer build bullet with the reproduction block so it uses `uv run maturin develop --release --manifest-path rust/cuprum-rust/Cargo.toml` from the repository root.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (1 warning)
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Replace the hardcoded `.venv/bin/python` in the read-size sweep snippet with `uv run python` to match the portable entrypoint used elsewhere in the document. Document the py-spy corroboration step so the parent-only folded captures under `dist/profiles/pyspy/` can be regenerated. py-spy must attach to the worker interpreter directly (not `uv run` or the writer subprocess), so resolve the venv interpreter portably via `sys.executable` rather than a hardcoded path.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
The Kani `LD_LIBRARY_PATH` example used a hardcoded personal home directory (`/home/leynos/.kani/...`), exposing a username. Replace it with `$HOME` and derive the version directory from `cargo kani --version` so the command works on any installation. The path predates this branch on `origin/main`; this commit fixes it while the surrounding profiling documentation is in flight.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? docs/tee-hotpath-profiling-baseline-2026-06-12.md (2)
♻️ Proposed fix-.venv/bin/python -c "
+uv run python -c "🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check lines 70-95 for the sweep driver snippet
echo "=== Lines 70-95 ==="
sed -n '70,95p' docs/tee-hotpath-profiling-baseline-2026-06-12.md
echo ""
echo "=== Lines 200-253 (reproduction section) ==="
sed -n '200,253p' docs/tee-hotpath-profiling-baseline-2026-06-12.md
echo ""
echo "=== Search for .venv/bin/python ==="
grep -n '\.venv/bin/python' docs/tee-hotpath-profiling-baseline-2026-06-12.md || echo "No .venv/bin/python found"
echo ""
echo "=== Search for py-spy invocation ==="
grep -n 'py-spy\|pyspy' docs/tee-hotpath-profiling-baseline-2026-06-12.mdLength of output: 5199
No further work is required. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/developers-guide.md (1)
145-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the writable
sysctlform here.
sysctl kernel.perf_event_paranoid=2only reads the value; it does not change
the kernel setting. Replace it withsudo sysctl -w kernel.perf_event_paranoid=2
so the documented fallback actually works.🔧 Proposed fix
- lower it for the session (`sudo sysctl kernel.perf_event_paranoid=2`, or a + lower it for the session (`sudo sysctl -w kernel.perf_event_paranoid=2`, or aRun this read-only check to confirm the local
sysctlbuild exposes the write
flag before publishing the example:#!/bin/bash set -euo pipefail sysctl --help | grep -E -- '(^|[[:space:]])-w([[:space:]]|,|$)'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/developers-guide.md` around lines 145 - 152, The documentation for lowering perf_event_paranoid contains an incomplete sysctl command that only reads the current value but does not modify it. In the section describing the fallback option to lower perf_event_paranoid for the session, update the command from `sudo sysctl kernel.perf_event_paranoid=2` to `sudo sysctl -w kernel.perf_event_paranoid=2` by adding the `-w` flag so the documented command actually writes and applies the kernel setting change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@docs/developers-guide.md`:
- Around line 145-152: The documentation for lowering perf_event_paranoid
contains an incomplete sysctl command that only reads the current value but does
not modify it. In the section describing the fallback option to lower
perf_event_paranoid for the session, update the command from `sudo sysctl
kernel.perf_event_paranoid=2` to `sudo sysctl -w kernel.perf_event_paranoid=2`
by adding the `-w` flag so the documented command actually writes and applies
the kernel setting change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f66dd09-e4bf-425e-905c-88be0190155c
📒 Files selected for processing (2)
docs/developers-guide.mddocs/tee-hotpath-profiling-baseline-2026-06-12.md
Add the `-w` flag to the documented `sudo sysctl kernel.perf_event_paranoid=2` fallback so the command explicitly writes the kernel setting.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
State the perf remediation precisely: `sysctl -w` changes a host-level kernel setting (not a session-local one), and `CAP_PERFMON` is granted to the `perf` binary (for example with `setcap`), not to an invocation. Prefix the Kani example with `cd rust` so it resolves the `cuprum-rust` package from the Rust workspace when run from the repository root, as every Makefile cargo target already does.
|
@coderabbitai Have the following now been resolved? docs/developers-guide.md (2)
🛠️ Proposed fix- lower it for the session (`sudo sysctl -w kernel.perf_event_paranoid=2`, or a
- lower value such as `1` or `-1` to resolve kernel frames too) or grant
- `CAP_PERFMON` to the `perf` invocation.
+ lower it on the host (`sudo sysctl -w kernel.perf_event_paranoid=2`, or a
+ lower value such as `1` or `-1` if kernel frames are also required), or
+ grant `CAP_PERFMON` to the `perf` binary.Based on the benchmark prerequisites and profiler invocation notes, the remediation path needs to be stated as a host-level setting change or a binary capability grant. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In
🛠️ Proposed fix-LD_LIBRARY_PATH="$HOME/.kani/kani-$(cargo kani --version | awk '{print $2}')/toolchain/lib" \
- cargo kani --package cuprum-rust
+cd rust && \
+LD_LIBRARY_PATH="$HOME/.kani/kani-$(cargo kani --version | awk '{print $2}')/toolchain/lib" \
+ cargo kani --package cuprum-rustBased on the repository layout and Rust testing guidance, the example needs the 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check lines 140-160 for the perf permission workaround
echo "=== Lines 140-160 (perf permission section) ==="
sed -n '140,160p' docs/developers-guide.md
echo ""
echo "=== Lines 485-505 (Kani example section) ==="
sed -n '485,505p' docs/developers-guide.md
echo ""
echo "=== Search for 'for the session' ==="
grep -n 'for the session' docs/developers-guide.md || echo "No match"
echo ""
echo "=== Search for 'CAP_PERFMON' ==="
grep -n 'CAP_PERFMON' docs/developers-guide.md || echo "No match"
echo ""
echo "=== Search for 'cd rust' or 'rust/' near cargo kani ==="
grep -n 'cd rust\|rust/\|cargo kani' docs/developers-guide.md || echo "No match"Length of output: 3632
No further work is required. |
2 participants
Summary
This branch records the first full run of the tee hot-path profiling
harness as the ADR-002 Phase 1 baseline evidence. It adds a findings
document with measured hotspot verdicts for the seven-scenario matrix,
run on commit
a50a631withperf(DWARF call graphs, 999 Hz) andpy-spy corroboration against the deterministic seed-12345 fixtures
(1.5 GiB raw). No production code changes are included.
Relates to #127: the evidence supports wiring capture-only consume
dispatch through
rust_consume_stream(ADR-002 Phase 2), the work thatPR #164 annotates as deferred pending exactly this measurement. The
issue remains open; this branch supplies the evidence gate only.
Key verdicts:
bytearray.extendplus finalbuffer.decode)is ~51% of parent CPU in tee mode — comfortably above the ADR-002
20% acceptance threshold for Phase 2;
(~46× slowdown), driven by per-line
ExecEventconstruction,inspect.isawaitable, andargv_with_programrecomputation, not byline splitting or incremental decode;
_READ_SIZEfrom 4 KiB to 16–64 KiB is a cheap ~20% win;splicepump reduces multi-stage pumping cost from ~36 sto ~1 s;
Two incidental findings are recorded for follow-up issues: a silent
FD-close race on the Rust pump path (
OSError: [Errno 9]duringtransport shutdown, exit code still 0) and a distro
perf scriptquirk that drops Python JIT frames from the folded stacks while
perf reportresolves them.Review walkthrough
docs/tee-hotpath-profiling-baseline-2026-06-12.md
for the environment, scenario results, the six hypothesis verdicts,
incidental findings, and the implications for ADR-002 phasing.
docs/contents.md
for the documentation index entry.
Validation
make markdownlint nixie: 0 errors, all diagrams validatedmake check-fmt: cleanmake fmt: applied (mdformat reflowed the new document)document; raw artefacts live under the gitignored
dist/profiles/Notes
The harness itself (merged in #52) needed no changes. The read-size
sweep monkeypatched
cuprum._streams._READ_SIZEin a throwaway driverrather than altering the harness; the document marks that as a
candidate ADR-002 Proposal 3 change.
Summary by Sourcery
Document tee hot-path profiling baseline measurements as ADR-002 Phase 1 evidence without changing production code.
Documentation: