Skip to content

observe(storage): measure effective cores per lifecycle region (#1462) - #1470

Merged
DecisionNerd merged 1 commit into
mainfrom
observe/1462-effective-cores
Sep 18, 2026
Merged

DecisionNerd merged 1 commit into
mainfrom
observe/1462-effective-cores

Conversation

@DecisionNerd

@DecisionNerd DecisionNerd commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Why

#1387 budgets a serialized fraction of the ingest path, and nothing on that path computes one. effective_cores existed only in benches/m6_storage_io.rs, so no receipt, ladder rung or test could report it — every "ingest is ~68–80% serial" figure in the plan is inferred from phase totals rather than measured.

What it adds

concurrency_attribution: elapsed wall against process CPU for a region, effective_cores() as their ratio, and serial_fraction() inverting Amdahl for a known worker count.

It does not touch storage_attribution.rs

Worth stating, because it was expected to. #1462 was sequenced to stack on PR #1415 on the assumption it would extend the I/O-attribution surface, which #1415 is rewriting.

It doesn't need to. Effective cores is a timing measurement, not an I/O-attribution one, so it lives in its own module and shares no file with #1415. The stacking constraint is dissolved — these two can land in either order.

That also serves the engine-independence requirement: regions are timed at their boundaries through measure and RegionScope, reaching into no partitioner or spill internal, so the instrument survives the engine replacement in #1456 instead of dying with the code it instruments.

/proc/self/stat, not getrusage

This crate is #![forbid(unsafe_code)], which cannot be locally overridden, so the FFI call the bench uses is unavailable here. utime + stime in /proc/self/stat is process-wide across threads exactly as RUSAGE_SELF is.

Fields are counted from the last ), because the comm field can contain spaces and parentheses. A test feeds it 42 (od ) (d :) name) S ... — a naive split_whitespace reads the wrong columns and reports a plausible-but-wrong CPU time, which is precisely the silent-under-reporting failure #1449 exists about.

Two things report None rather than a number: CPU on a platform without /proc, and a serial fraction that doesn't fit the model (one worker, non-positive speedup, or superlinear). A fabricated zero would be indistinguishable from a measurement.

Calibrated against a known positive

#1462 requires it, and the brief requires it, for a good reason: without both ends, a low reading from real ingest cannot be told apart from an instrument that always reads low.

region measured
one busy thread ~1 effective core
four busy threads ~4 effective cores
sleeping < 0.5
cargo test -p graphforge-storage --lib concurrency_attribution -- --ignored --test-threads=1

Writing the calibration established a constraint worth knowing

Those three are #[ignore]d, and that is not convenience. Process CPU is process-wide, so under cargo test's default parallelism a region that does nothing but sleep measured 4.9 effective cores — other tests' work, counted correctly, answering a different question.

That is the right semantics for the purpose: a ladder rung runs one phase per single-purpose process invocation, and the question is how much of the machine that phase used. But it means the calibration cannot be a default-parallel gate, and it means anyone reading effective cores for a region must know the process was doing only that. The module documents it; this is the same shape as #1460.

Verification

  • graphforge-storage lib: 1174 passed / 0 failed, 5 ignored.
  • Calibration in a quiet process: 3 passed / 0 failed.
  • cargo clippy --workspace -- -D warnings (CI's exact invocation): exit 0.
  • cargo fmt --all -- --check: clean. make pre-push-fast: passed.

Not in this PR

Wiring RegionScope into the ingest phases, and surfacing the figures on the receipt and in the ladder rung, so the budget can actually be read off a run. That is the next slice; this one establishes a measurement that can be trusted first.

Refs #1462

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

#1387 budgets a serialized fraction of the ingest path and nothing on that path
computes one. `effective_cores` existed only in `benches/m6_storage_io.rs`, so
no receipt, ladder rung or test could report it, and every "ingest is ~68-80%
serial" figure in the plan is inferred from phase totals rather than measured.

Adds `concurrency_attribution`: elapsed wall against process CPU for a region,
`effective_cores()` as their ratio, and `serial_fraction()` inverting Amdahl for
a known worker count.

Deliberately its own module, measuring at region boundaries through `measure`
and `RegionScope`. It reaches into no construction internal, so it survives the
engine replacement in #1456 rather than dying with the code it instruments --
and it does not touch `storage_attribution.rs`, which PR #1415 is rewriting.

`process_cpu_time` reads `/proc/self/stat` rather than calling `getrusage`,
because this crate is `#![forbid(unsafe_code)]`. `utime + stime` there is
process-wide across threads exactly as `RUSAGE_SELF` is. Fields are counted from
the last `)`, since the comm field can contain spaces and parentheses; a test
feeds it `(od ) (d :) name)` because a naive split would silently report a
plausible wrong value.

Two things are reported as `None` rather than as a number: CPU on a platform
without `/proc`, and a serial fraction that does not fit the model. A fabricated
zero would be indistinguishable from a measurement.

**Calibrated against a known positive**, which #1462 requires: one busy thread
measures ~1 effective core, four measure ~4, and a sleeping region measures
<0.5. Without both ends, a low reading from real ingest cannot be told apart
from an instrument that always reads low -- #1449 is the standing example.

Those three calibrations are `#[ignore]`d and must run in a quiet process:

    cargo test -p graphforge-storage --lib concurrency_attribution \
        -- --ignored --test-threads=1

Writing them established why. Process CPU is process-wide, so under `cargo
test`'s default parallelism a region that does nothing but sleep measured **4.9
effective cores** -- other tests' work, correctly counted, answering a different
question. That is the right semantics for the purpose, since a ladder rung runs
one phase per single-purpose process, but it means the calibration cannot be a
default-parallel gate. Same shape as #1460. The module says so.

Verified: `graphforge-storage` lib 1174 passed / 0 failed; calibration 3 passed
in a quiet process; `cargo clippy --workspace -- -D warnings` exit 0; fmt clean;
`make pre-push-fast` passed.

Refs #1462

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. 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: d1326f50-534a-4ad0-9195-4590dfd6d560

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

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


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

core Core source code changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant