Skip to content

fix(perf): the memory number carries the name of what it measures (S8) - #355

Open
PhysShell wants to merge 3 commits into
claude/p-022-analysis-wiring-mwhqlwfrom
claude/p022-s8-memory-metric
Open

PhysShell wants to merge 3 commits into
claude/p-022-analysis-wiring-mwhqlwfrom
claude/p022-s8-memory-metric

Conversation

@PhysShell

Copy link
Copy Markdown
Owner

S8 from the hostile freeze review of #354. Instrument semantics repair — no policy, no threshold, no authorization.

What a witness established

peak_rss_bytes was false on Windows, and the witness settles it rather than a manual page.

Windows, job object as the harness uses it (CreateJobObjectW + AssignProcessToJobObject):
  child writes 128 MiB                      PeakProcessMemoryUsed  134.3 MiB
  child COMMITS 256 MiB and touches nothing PeakProcessMemoryUsed  262.5 MiB   <-- not resident
  light control                             PeakProcessMemoryUsed    7.3 MiB

Linux, os.wait4 on the immediate child as the harness uses it:
  heavy IS the waited child                 ru_maxrss  260.2 MiB
  shell -> heavy child                      ru_maxrss  260.2 MiB
  shell -> light -> heavy (production shape) ru_maxrss  260.2 MiB
  shell -> shell -> heavy (grandchild)      ru_maxrss  260.3 MiB
  light shell only                          ru_maxrss   13.3 MiB

The job object counts committed memory — job and process memory limits are defined on committed virtual memory — while ru_maxrss counts resident pages. One field name claimed both.

The same witness refuted the other half of the suspicion: wait4 does not stop at a wrapping shell. The POSIX path already measures the tree it should.

The repair

The quantity now travels with the number. Every sample and every cell carries memory_metric from a closed set:

  • max_process_peak_resident — POSIX wait4, and the /usr/bin/time -v fallback
  • max_process_peak_commit — the Windows job object

A value produced under any other kind raises InstrumentError rather than being recorded. No downstream reader infers semantics from sys.platform, and no alias lets Windows commit go on being read as RSS.

Renamed because the old nouns lied the same way: peak_rsspeak_memory, rss_mechanismmemory_mechanism, rss_unavailable_reasonmemory_unavailable_reason, raw_peak_rss_bytesraw_peak_memory_bytes, RssProbeMemoryProbe.

Deliberately not done: Windows resident-set sampling. PeakWorkingSetSize is the resident analogue, but obtaining it for a tree of processes that come and go needs handle tracking or polling. This instrument does not sample, and will not perturb the elapsed-time interval so two operating systems can pronounce the same noun.

The digest moved, on purpose

562a7f7232dad2f4c79c6adfe0e1e7e25680b4b6f3444d54824bf0405e3c14b3   before
104c384d01bf6060bdec1e7c916053ddb04b97fcbd0b39f8a4fc57b8f139672f   after

Three controls said so by name — freeze-harness-untouched, training-prereg-bindings, envcapture-frozen-untouched — which is the provenance system delivering its inconvenience on purpose. Steps 4/5/6 are re-bound in their own commit (including design_constants_blob_sha1, which moved because the design-constants artifact was itself re-bound).

Committed evidence of runs that actually happened is not rewritten. The sizing, calibration and round-7 datasets record what the old instrument produced and still carry peak_rss; rewriting them would forge a record rather than re-accept a binding.

Controls

calibration constants 4/4 · calibration freeze 7/7 · calibration policy 10/10 · perf instrument 16/16 · round 7 apparatus 10/10 · step 7 environment capture 11/11 · training preregistration 9/9 · evidence coverage 12/12.

The repository runner also reports 17 FAIL[...] lines from variant-corpus, variant-ledger and artifact-golden. Those are pre-existing on this Windows checkout — the same 7 + 10 appear at the base commit 6194d7e — and are byte-exact fixtures against a CRLF working tree, unrelated to this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_018xhcg5opoFbSTdYHpkSXCh

`peak_rss_bytes` was false on Windows. A witness settles it rather than a manual
page: a child that commits 256 MiB and never touches a page is reported as
262.5 MiB by the job object's PeakProcessMemoryUsed, while on Linux nothing
becomes resident. The job object counts COMMITTED memory — job and process
memory limits are defined on committed virtual memory — and `ru_maxrss` counts
RESIDENT pages. One field name claimed both.

The same witness refuted the other half of the suspicion. `wait4` on the
immediate child does NOT stop at the shell: shell -> heavy child and
shell -> shell -> heavy child both return the descendant's 260 MiB against
13 MiB for a light control, so the POSIX path already measures the tree it
should.

So the quantity now travels with the number. Each sample and each cell carries
`memory_metric`, drawn from a closed set — `max_process_peak_resident` on the
wait4 and `/usr/bin/time -v` paths, `max_process_peak_commit` on the Windows
job object — and a value produced under any other kind raises rather than being
recorded. No downstream reader infers semantics from `sys.platform`, and no
alias lets Windows commit go on being read as RSS.

Renamed with it, because the old nouns lied in the same way: `peak_rss` ->
`peak_memory`, `rss_mechanism` -> `memory_mechanism`, `rss_unavailable_reason`
-> `memory_unavailable_reason`, `raw_peak_rss_bytes` -> `raw_peak_memory_bytes`,
`RssProbe` -> `MemoryProbe`.

Deliberately NOT done: Windows resident-set sampling. `PeakWorkingSetSize` is
the resident analogue, but obtaining it for a tree of processes that come and go
needs handle tracking or polling, and this instrument does not sample. Two
operating systems answering different, honest questions beats a profiler added
so they can pronounce the same noun.

This moves `measurement_harness_digest` 562a7f7232da -> 104c384d01bf. The
bindings that name the old digest are re-bound in the next commit; nothing here
tries to preserve it.

perf instrument controls 16/16, round 7 apparatus controls 10/10.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018xhcg5opoFbSTdYHpkSXCh
The memory-semantics repair moved `measurement_harness_digest`

    562a7f7232dad2f4c79c6adfe0e1e7e25680b4b6f3444d54824bf0405e3c14b3   before
    104c384d01bf6060bdec1e7c916053ddb04b97fcbd0b39f8a4fc57b8f139672f   after

and three controls said so, each by name: `freeze-harness-untouched`,
`training-prereg-bindings` and `envcapture-frozen-untouched`. That is the
provenance system delivering its inconvenience on purpose, so nothing here tries
to preserve the old digest.

Re-bound: the digest in the policy freeze, in the ratified design constants, and
in the training preregistration's bindings — and with it
`design_constants_blob_sha1`, which moved because the design-constants artifact
itself was re-bound. A chain of bindings re-bound in the order the chain runs.

Not touched: the committed evidence of runs that actually happened. The sizing,
calibration and round-7 datasets were produced by the old instrument and record
what it produced; rewriting them would be forging a record, not re-accepting a
binding. They carry `peak_rss` because that is the field the instrument had when
they were taken.

This is a re-acceptance of the step-4/5/6 bindings, not a code change; it is a
separate commit so it can be reviewed as one. Controls after re-binding:
calibration constants 4/4, calibration freeze 7/7, calibration policy 10/10,
perf instrument 16/16, round 7 apparatus 10/10, step 7 environment capture
11/11, training preregistration 9/9.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018xhcg5opoFbSTdYHpkSXCh
The section was called "Peak RSS" and described two mechanisms as though they
produced one number. They do not: POSIX `ru_maxrss` is a peak resident set over
the waited-for descendant chain, and the Windows job object's
`PeakProcessMemoryUsed` is peak committed memory over the processes of the job.
The witness that settles it is recorded here rather than left in a session log —
256 MiB committed and never touched reads as 262.5 MiB on Windows and as nothing
resident on Linux — as is the half of the suspicion the same witness refuted:
`wait4` does not stop at a wrapping shell.

Recorded with it: the strata answer different honest questions and may not be
pooled across platforms; Windows resident sampling was considered and refused
because this instrument does not sample; and the digest moved 562a7f7232da ->
104c384d01bf, with steps 4/5/6 re-bound in their own commit and the committed
evidence of past runs deliberately left alone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018xhcg5opoFbSTdYHpkSXCh
@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 21703a95-7c8e-40bd-a662-d05dd2f834b3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

2 participants