Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3dd8c2e180
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| CPU_STAT="${CGROUP_ROOT:-/sys/fs/cgroup}${container_cgroup}/cpu.stat" | ||
| grep -q '^usage_usec ' "$CPU_STAT" 2>/dev/null \ |
There was a problem hiding this comment.
When Podman reports an empty or / State.CgroupPath, such as when no per-container cgroup is available, this resolves to the root cgroup's cpu.stat. That file normally contains usage_usec, so the guard passes and every request is charged CPU from the entire shared host rather than from the container, corrupting the host-control comparison. Validate that the path is absolute, nonempty, and not / before reading it.
AGENTS.md reference: bench/chromium/AGENTS.md:L336-L338
Useful? React with 👍 / 👎.
| block = { | ||
| "total": metric_summary(by[a], total), | ||
| "children_at_kill": metric_summary(by[a], parts("children")), | ||
| "reclaim": metric_summary(by[a], parts("reclaim")), | ||
| "memory_server": metric_summary(by[a], parts("serve")), |
There was a problem hiding this comment.
On normal CLK_TCK=100 runs, the child-at-kill, reclaim, and memory-server values are all derived from tick-quantized /proc/<pid>/stat readings, but this reduction sums their point values and publishes only a bootstrap CI. It even discards the existing reclaim *_lo/*_hi bounds, so request_cpu_ms.total can imply sub-tick precision and support differences smaller than the measurement resolution. Propagate counter bounds into the total or explicitly publish the accumulated quantization interval.
AGENTS.md reference: bench/chromium/AGENTS.md:L729-L732
Useful? React with 👍 / 👎.
…trol The corpus runs published no CPU figure. The fast-teardown arm already read each clone child's utime+stime at the kill and its reaping CPU after it, but nothing reduced them, the shared memory server's CPU was attributed to no request, and the host-container control recorded no CPU at all. bench/chromium/reqbench.py: - serve_cpu(): the memory server's utime+stime between a clone's launch and the end of its teardown. Requests run one at a time, so that delta is the request's share. A server whose start time changed, or that cannot be read, is an error, not zero; a file-backed restore has no server. - run_cdp_request records it as serve_cpu on every completed request. bench/chromium/reqanalyze.py: - arms[<fast arm>].request_cpu_ms: bootstrap medians of the total (children at the kill + their reaping + the memory server), of each part, and of each child at the kill, plus a count of totals that include a reaping the reaper raced (a lower bound). Records missing any part are left out. bench/chromium/reqscale.py: - Its requests overlap, so the memory server's CPU over one request's window is not that request's; reqscale drops serve_cpu from its records. bench/chromium/hostcdp.sh: - Reads the container cgroup's cpu.stat usage_usec before and after each drive() and records both counters on the rep. A container without the counter is refused (exit 5), as is a rep whose counter cannot be read. - summary.json container_cpu_ms: the median and mean per drive() window, and the run average, everything from the first measured rep's start to the last one's end divided by the measured count. A clone's CPU is its whole one-request life, so the run average is the comparable figure. - CGROUP_ROOT (default /sys/fs/cgroup) locates the cgroup; tests point it at a fixture. Tests, each observed failing on the parent: - ServeCpu (4 tests) and serve_cpu on run_cdp_request's record. - AnalyzerAvailability.test_request_cpu_sums_children_reaping_and_the_memory_server - HostCdpContainerCpu: the per-rep counters and summary, and the refusal. - ConcurrentRequestRecords: reqscale records carry no serve_cpu. - The hostcdp harnesses' podman stubs answer .State.CgroupPath and their stub drivers advance a fixture cpu.stat by 2.5 ms per call. Tested: python3 -m unittest discover -s bench/chromium -p 'test_*.py' (1060 tests, OK, before the reqscale change); test_reqscale (OK) after it.
… the run, fail closed From a Codex review of the previous commit. Per-request readings of the memory server and of utime+stime missed work in two ways, and the reducer published what it could read instead of refusing what it could not. bench/chromium/reqbench.py: - proc_cpu_ticks() adds cutime and cstime. The fast reap records them per pinned process as reaped_children_cpu_ms_by_child: fcvm's cp --reflink, nsenter and ip setup helpers have exited by the kill and were counted nowhere. - The memory server is sampled cumulatively (its own and its reaped children's CPU, with its start time) before each request's launch and after its teardown, as serve_cpu_before / serve_cpu_after. The per-request delta is gone: the server finishes working-set publication and copy-mode warming after a clone exits, outside that clone's window, and CLK_TCK steps made a 4 ms/request server read 0 most of the time. bench/chromium/reqanalyze.py: - request_cpu_ms reports means with a bootstrap CI (mean_ci), the same statistic as the host control's run average: the clone's tree (children at the kill + their reaped children + reaping), its parts, each child, and the count of totals that include a lower-bound reaping. - memory_server_average(): the server's cumulative growth from the first measured request to the last, divided by the count. Only when the run had one arm; a server shared with other arms, restarted, or missing a sample is reported as not attributable, and the total is then not published. - One measured fast request without a complete reading withholds every CPU figure (complete: false, missing_records) instead of publishing the rest under a smaller n. bench/chromium/hostcdp.sh: the container's cgroup path must be below the root, canonical, and contain the container's main pid (podman inspect .State.Pid) in its subtree. An empty path or "/" made the counter the root cgroup's, every process on the machine. bench/chromium/reqscale.py: drops the two server samples from its overlapping requests' records. Tests, each failing on the previous commit: - ServeCpuSample (3), MemoryServerAverage (5) - AnalyzerAvailability: test_request_cpu_counts_reaped_helpers_and_measured_lower_bounds (the lower-bound row is now a measured one) and test_one_incomplete_request_withholds_every_cpu_figure - TeardownFastReapGuard.test_a_reaped_helpers_cpu_is_recorded: a child burns 0.3 s in a helper it reaps, then execs sleep - CdpFailureIsLabelledOnTheRecord: the record's two server samples - HostCdpContainerCgroup (3): root/empty, climbing, and pid-less cgroups - HostCdpCpuSummary: the run average counts CPU between requests (a stronger check of an unchanged formula) Tested: python3 -m unittest discover -s bench/chromium -p 'test_*.py' (1073 tests, OK).
3dd8c2e to
5404b32
Compare
This branch has not been deployed
1 participant
Stacked on: bench/hostcdp-in-process (#1046)
The render benchmark page says CPU per request is not measured. The fast-teardown arm already read each clone child's utime+stime at the kill, and the CPU each child spent being reaped after it. Nothing reduced those readings, and nothing attributed the shared memory server's CPU to a request. The host-container control recorded no CPU at all.
Contract:
reqbenchrecords carry the memory server's CPU for each request.reqanalyzepublishes CPU per request for the fast-teardown arm.reqscalerecords carry no per-request server CPU.Downstream impact: new fields in records and summaries, and the host control now refuses a container without a CPU counter. The latency fields are unchanged.
Changes
reqbench.pyserve_cpu()takes the memory server's utime+stime between a clone's launch and the end of its teardown. Requests run one at a time, so that delta is the request's share.reqanalyze.pyarms[<fast arm>].request_cpu_msgives bootstrap medians of the total, which is the children at the kill plus their reaping plus the memory server.reqscale.pyserve_cpufrom its records.hostcdp.shcpu.statusage_usecbefore and afterdrive().container_cpu_ms: the median and mean over eachdrive()window, and the run average.CGROUP_ROOTlocates the cgroup, and tests point it at a fixture.Evidence
Each new test was observed failing on the parent:
ServeCpu(4 tests), andserve_cpuonrun_cdp_request's record.AnalyzerAvailability.test_request_cpu_sums_children_reaping_and_the_memory_serverHostCdpContainerCpu(the counters and summary, and the refusal).ConcurrentRequestRecords.test_the_memory_server_cpu_is_not_charged_to_overlapping_requestsThat run preceded the
reqscalechange;test_reqscalepasses after it.