Add windows-placement-probe, the NUMA placement measurement tool by MikeGrier · Pull Request #63 · MikeGrier/windows-threadpool-sys

MikeGrier · 2026-09-05T16:52:46Z

Fourth peel from #56, and the first that takes a whole crate cleanly.

Adds windows-placement-probe: a tool that measures what cross-domain
placement costs on the machine it runs on, rather than inferring it from the
topology description. It pins a producer and a consumer to chosen processors,
transfers through a windows-waitable-queues ring, and reports the difference
between same-domain and cross-domain placement, with the machine identified well
enough to compare runs against each other.

Unblocked by #61. Both in-repo dependencies -- windows-topology-sys 0.2.0 and
windows-waitable-queues 0.1.0 -- are now released on main, so this peels out
whole, borrowing nothing from crates still on #56.

Not published, on purpose

publish = false, so there is no publish-workflow or release-please wiring to
add. That is the crate's own decision, recorded in its DESIGN-NOTES: it is a
measurement instrument whose output describes one specific machine, not a
library anything links against. It ships through release-placement-probe.yml,
which builds the binary on tag.

Worth stating plainly because the reverse mistake -- a crate that is
release-managed but unpublishable -- is one this repository has actually made,
and #56 carries the CI check that would have caught it. That check is not in
this PR; see below.

What rides along, and why

The workspace member entry. There is no default-members list here, so
this also puts the crate into every unscoped cargo invocation, which is how
the existing CI jobs pick it up.
release-placement-probe.yml -- new, referenced by nothing else.
One ci.yml job, placement-probe-no-serde. The serde feature is
optional and the manifest promises the crate is usable without it, but no
--workspace step ever builds that configuration: they take the default
features, and --all-features turns it back on. Unprotected it did not merely
rot -- it never worked. paste_json and submission imported serde while
ungated, so the advertised configuration failed to compile outright with five
errors.

Only that one job was taken. ci.yml on the source branch also gains a
publishable check, NUMA spike steps, and probe-magnitude steps -- each
depending on scripts or crate changes still on #56. Bringing them would be a
second change wearing this one's clothes.

Documentation links

Two DESIGN-NOTES.md links point at CHECKLIST-placement-tool.md, a
workspace-root file that stays on #56. Link text kept, target removed.

Bringing that checklist instead was measured rather than guessed: it would
resolve those two links and introduce six new dangling ones of its own. Same
test, same verdict as topology-planner in #61.

Verification

256 tests with all features, 196 with none
both configurations clean under cargo clippy -- -D warnings
both clean under CI's rustdoc flags -- checked in both configs deliberately,
since a link into a feature-gated item passes --all-features and fails the
default build
cargo check --workspace --all-targets --all-features clean
cargo fmt --all --check exit 0; encoding check 568 files clean
every relative documentation link in the changed files resolves (2 resolve, 0
dangle)
both edited workflow files parse as YAML

Peel sequence

With this merged, #56 drops from 167 files to 133. windows-platform-probes
(34 files) is the next candidate -- it depends on this crate, and its other
dependencies are already on main.

A new crate that measures what cross-domain placement actually costs on the machine it runs on, rather than inferring it from the topology description. It pins a producer and a consumer to chosen processors, transfers through a `windows-waitable-queues` ring, and reports the difference between same-domain and cross-domain placement with the machine identified well enough to compare runs. Unblocked by #61: every in-repo dependency -- `windows-topology-sys` 0.2.0 and `windows-waitable-queues` 0.1.0 -- is now released on main, so this peels out of #56 whole, with nothing borrowed from crates still on that branch. `publish = false`, so it needs no entry in the publish workflow or the release-please configuration -- deliberately, per the crate's own DESIGN-NOTES: it is a measurement instrument whose output is a record of a specific machine, not a library anything links against. It ships instead through `release-placement-probe.yml`, which builds the binary on tag. Three things ride along because the crate does not work without them: - The workspace member entry. There is no `default-members` list here, so this also puts the crate in every unscoped `cargo` invocation, which is where CI picks it up. - `release-placement-probe.yml`, new, and referenced by nothing else. - One `ci.yml` job, `placement-probe-no-serde`. The `serde` feature is optional and the manifest promises the crate works without it, but no `--workspace` step ever builds that configuration: they take the default features, and `--all-features` turns it back on. Unprotected it did not merely rot, it never worked -- `paste_json` and `submission` imported `serde` while ungated, so the advertised configuration failed to compile with five errors. Only that one job was taken. `ci.yml` on the source branch also gains a `publishable` check, NUMA spike steps, and probe-magnitude steps, all of which depend on scripts or crate changes that are still on #56; bringing them would be a second change wearing this one's clothes. Two `DESIGN-NOTES.md` links point at `CHECKLIST-placement-tool.md`, a workspace-root file that stays on #56. Link text kept, target removed. Bringing the checklist instead was measured and rejected: it would resolve those two and introduce six new dangling links of its own. Verified: 256 tests with all features and 196 with none, both configurations clean under `cargo clippy -D warnings` and under CI's rustdoc flags, `cargo check --workspace --all-targets --all-features` clean, `cargo fmt --all --check` exit 0, encoding check 568 files clean, and every relative documentation link in the changed files resolves. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

There is at least one confirmed functional issue in the new report renderer (report.rs) where an intended writeln! call is accidentally commented out.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds the windows-placement-probe crate: a Windows-only measurement tool that times cross-domain thread/memory placement costs, produces a paste-ready submission record (with checksum + schema guard), and ships as CI-built release binaries via a dedicated tag workflow. This peel also wires the crate into the workspace and adds CI coverage for the advertised --no-default-features (no serde) configuration.

Changes:

Add the windows-placement-probe crate (measurement, record/report rendering, redaction policy, paste JSON layout, schema golden, sabotages, and CLI binary).
Add a dedicated release-placement-probe GitHub Actions workflow to build/tag-release and attach attested binaries.
Update workspace/CI to include the new crate and to build/test the no-serde configuration.

File summaries

File	Description
crates/windows-placement-probe/Cargo.toml	New crate manifest; `publish = false`, calendar versioning, feature gating (`serde`) and binary `required-features`.
crates/windows-placement-probe/README.md	End-user documentation for running the probe, what it collects, and provenance verification guidance.
crates/windows-placement-probe/DESIGN-NOTES.md	Design rationale/decisions for the placement probe (new component notes).
crates/windows-placement-probe/build.rs	Build stamping logic for commit/dirty/source plus git-ref watching for local rebuild correctness.
crates/windows-placement-probe/sabotage.json	Sabotage test definitions to enforce redaction/policy invariants and output guarantees.
crates/windows-placement-probe/schema/v1.txt	Schema golden for record shape v1 (append-only from first release).
crates/windows-placement-probe/src/lib.rs	Crate root, module layout, and feature-gating for serialization-dependent modules.
crates/windows-placement-probe/src/build_identity.rs	Build identity model (source/commit/dirty) and official-build predicate/rendering.
crates/windows-placement-probe/src/build_identity/tests.rs	Tests for build identity semantics and build-script stamping behavior.
crates/windows-placement-probe/src/core_affinity.rs	Core placement experiment and run planning/measurement logic.
crates/windows-placement-probe/src/core_affinity/tests.rs	Tests for placement experiment planning/selection and measurement invariants.
crates/windows-placement-probe/src/fingerprint.rs	Host fingerprint and processor “place” modeling used for labeling and validation.
crates/windows-placement-probe/src/fingerprint/tests.rs	Tests for fingerprint formatting, semantics, and cache/NUMA handling.
crates/windows-placement-probe/src/machine.rs	Machine-description collection (CPU model, OS build, virtualization hint) gated by policy.
crates/windows-placement-probe/src/machine/tests.rs	Tests for machine-description policy, registry read behavior, and virtualization classification.
crates/windows-placement-probe/src/paste_json.rs	Deterministic, width-aware JSON layout preserving declaration field order.
crates/windows-placement-probe/src/paste_json/tests.rs	Tests for JSON round-trip, width rules, and field-order preservation.
crates/windows-placement-probe/src/peer_index_cache.rs	Peer-index caching strategy implementation used by the measurement.
crates/windows-placement-probe/src/peer_index_cache/tests.rs	Tests for peer-index caching behavior and measurement expectations.
crates/windows-placement-probe/src/record.rs	Submission record model, timestamp flooring/redaction, and schema versioning; assembly from observation.
crates/windows-placement-probe/src/record/tests.rs	Tests for schema-guard behavior, timestamp semantics, and record assembly invariants.
crates/windows-placement-probe/src/redaction.rs	`MetadataPolicy` definition and semantics (default redaction, opt-in inclusion).
crates/windows-placement-probe/src/redaction/tests.rs	Tests for default policy behavior and inclusion/withholding semantics.
crates/windows-placement-probe/src/report.rs	Human-readable report rendering from the record (including provenance/disagreement messaging).
crates/windows-placement-probe/src/report/tests.rs	Tests for report content and rendering constraints.
crates/windows-placement-probe/src/submission.rs	Paste-ready submission formatting (markdown fences + checksum) and safe/predictable filename derivation.
crates/windows-placement-probe/src/submission/tests.rs	Tests for submission formatting, checksum correctness/stability, and filename safety/collision rules.
crates/windows-placement-probe/src/bin/placement_probe/main.rs	CLI entrypoint wiring policy, measurement, record creation, backup writing, and sink-based output.
crates/windows-placement-probe/src/bin/placement_probe/sink.rs	Output abstraction (stdout/stderr sink) and capture sink for tests.
crates/windows-placement-probe/src/bin/placement_probe/tests.rs	End-to-end-ish tests over CLI renderers/sink behavior.
Cargo.toml	Add `crates/windows-placement-probe` as a workspace member.
Cargo.lock	Lockfile update to include the new crate package entry.
.github/workflows/release-placement-probe.yml	New tag-based workflow to build/verify/artifact/attest and publish probe binaries.
.github/workflows/ci.yml	Add a job that builds/tests/docs the crate with `--no-default-features` (no `serde`).

Review details

Files reviewed: 33/34 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The `writeln!` separating the "What this does NOT establish" paragraph from whatever precedes it sat on the same line as the trailing `//` of the comment explaining it, so the compiler saw only a comment. The blank line never printed on any run, in either branch, and the caveat ran on from the line above -- which on the marked path is the end of the provenance list. Raised in the PR #63 review, and inherited from #56 rather than introduced by the peel; the peel copied the file unchanged. Covered by a test, written first and confirmed failing against the unfixed code. It asserts structure rather than prose, which is what this module's header says its tests are for: the existing `the_ordering_caveat_is_stated_even_on_a_clean_run` checks the paragraph is present, and passed throughout, because presence and separation are different properties. Both branches are asserted -- only the marked one puts visible text immediately above the caveat. Swept the crate for the same shape, a statement swallowed by a trailing `//` on a comment line: no other instance. Verified: 257 tests with all features and 197 with none, both one higher than before; both configurations clean under `cargo clippy -- -D warnings` and under CI's rustdoc flags; `cargo fmt --all --check` exit 0; encoding check clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

There are correctness/maintainability issues to address (notably a test that doesn’t actually assert its stated round-trip property, plus serde/serde_json feature wiring consistency).

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

crates/windows-placement-probe/Cargo.toml:100

serde_json is an unconditional dependency, which means --no-default-features still pulls in the serde stack via serde_json. If the intent is that the measurement-only build is usable without serde (including dependency footprint), consider making serde_json optional and enabling it via the existing serde feature alongside dep:serde.
crates/windows-placement-probe/src/record/tests.rs:308
The test claims consecutive days must differ by exactly one day, but it only asserts that no date repeats; an implementation could skip/duplicate days without repeating and still pass. Consider round-tripping by adding a local days_from_civil inverse and asserting days_from_civil(civil_from_days(day)) == day for the whole span.

Files reviewed: 33/34 changed files
Comments generated: 0 new
Review effort level: Lite

…ound trip Two findings from the PR #63 review, both confirmed by measurement before being fixed. `serde_json` was an unconditional dependency, and it depends on serde -- so `--no-default-features` switched off this crate's own derives while the stack the feature exists to avoid arrived through the side door. Measured with `cargo tree`: serde and serde_core were both in the no-default build before, and neither is now, while `--all-features` still carries both. Every use of `serde_json` was already behind `cfg(feature = "serde")` -- `paste_json`, `submission`, and the record's serialization tests -- so nothing outside the feature loses anything. `the_civil_conversion_round_trips_across_a_long_span` was named for a round trip it did not perform. It asserted that no date REPEATED, which is far weaker than its own comment claimed: a conversion that skipped a day, or shifted every date by one, repeats nothing and passed. It now round-trips through Howard Hinnant's `days_from_civil`, the counterpart algorithm to the `civil_from_days` under test -- the era arithmetic run in the opposite direction, so agreement is evidence about the conversion rather than about a copy of it. Verified by sabotage in both directions, because the whole finding is that a green test proved nothing. With `days + 1` injected into `civil_from_days`: the new assertion fails at day 0 naming 1970-01-02, and with only that assertion removed the old one passes on the same sabotaged code. The sabotage was then reverted and the test re-run clean. Verified: 257 tests with all features and 197 with none, both configurations clean under `cargo clippy -- -D warnings` and under CI's rustdoc flags, `cargo check --workspace --all-targets --all-features --locked` clean, `cargo fmt --all --check` exit 0, encoding check 568 files clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🟢 Approval recommended

The changes appear cohesive and well-tested, with only a minor build-script robustness tweak suggested.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/windows-placement-probe/build.rs:35

watch_git_head(Path::new("../../.git")) relies on the build script’s current working directory to resolve the relative path. Build scripts are usually executed with CARGO_MANIFEST_DIR as CWD, but making this resolution explicit avoids a silent no-op if that assumption ever changes, which would reintroduce stale commit stamps on local builds.

Files reviewed: 33/34 changed files
Comments generated: 0 new
Review effort level: Lite

…just keys Found by a code review of this branch. `every_field_of_a_fully_populated_record_is_present_in_the_json` could not fail. It checked six paths against `key_paths`, which inserts a field's path *before* it looks at the value -- and no field in this record carries `skip_serializing_if`, so a `None` serializes as an explicit `null` whose key is present either way. All six paths are `Option`s of scalars, so every assertion held whether the fixture populated the field or left it unset. Confirmed by sabotage: with `cpu_model: None` the old test passed. It now asserts over `populated_paths`, which skips a `null` and an empty list. Re-run under the same sabotage it fails, naming `machine.cpu_model`. The hazard it was written for is real but a different shape than the six paths it listed: a path leaves the golden when an `Option<Struct>` is `None` or a `Vec` is empty, because then the nested paths vanish rather than becoming null. The fixture handles both -- `Coherence::Disagreed`, and three non-empty measurement lists -- and the fixture's own comments promise it, but nothing checked. The list now covers them, and emptying `node_hops` fails the test naming `node_hops[].memory_node`. `key_paths` is unchanged and still derives the golden: a `null` key IS part of the serialized shape, so the golden should keep recording it. The two functions answer different questions, which is why this is a second walker rather than an edit to the first. Verified: 234 tests with all features and 197 with none, both configurations clean under `cargo clippy -- -D warnings`, `cargo check --workspace --all-targets --all-features --locked` clean, `cargo fmt --all --check` exit 0, encoding check 568 files clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

It introduces a new crate plus multiple workflows/release mechanics, so it warrants final human review despite only minor issues found.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

crates/windows-placement-probe/Cargo.toml:27

This comment references CHECKLIST-placement-tool.md, but that file is not present in this branch/repo (so the reference can’t be followed). Consider pointing readers to the in-repo design notes (or the PR/issue where the checklist lives) instead of naming a non-existent file.
crates/windows-placement-probe/DESIGN-NOTES.md:305
This references CHECKLIST-placement-tool.md, but that file is not present in this branch/repo, so readers can’t follow the provenance trail. It would be clearer to point to the PR where that checklist currently lives (or to another in-repo document) instead of naming a missing file.

This issue also appears on line 397 of the same file.

crates/windows-placement-probe/DESIGN-NOTES.md:401

This references CHECKLIST-placement-tool.md, but that file is not present in this branch/repo, so the reference can’t be followed. Consider pointing to the PR/issue where that checklist currently lives (or another in-repo doc) instead of naming a missing file.

Engineer's decision on the wording, 2026-09-04. Queued as `M36.4` in
CHECKLIST-placement-tool.md, completing M36.
Three of the nine sabotages in [sabotage.json](sabotage.json) cover this section:
printing it on every run, dropping its closing release, and advising
`--include-metadata` to a record that already carries it.

Files reviewed: 33/34 changed files
Comments generated: 0 new
Review effort level: Lite

CHECKLIST-placement-tool.md was never committed -- it is absent from the working tree, from the sibling worktree, and from every commit reachable in this repository. Three references named it as though a reader could open it: the registry-publication comment in Cargo.toml, and the M36.2 and M36.4 provenance lines in DESIGN-NOTES.md. Keep the work-item IDs, which are the provenance actually being cited, and drop the file name so nothing points at a missing target. The README mention alongside M36.3 becomes a real link while the sentence is being touched. No behavior change; comments and prose only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

build.rs currently relies on a relative ../../.git path that can silently break commit/dirty stamping if the build script CWD/layout differs, weakening the tool’s provenance guarantees.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/windows-placement-probe/build.rs:35

watch_git_head(Path::new("../../.git")) relies on the build script's current working directory. If Cargo ever runs the build script with a different CWD (or if this crate is built in a different layout), this will silently behave as “no repository” and stop refreshing the commit/dirty stamps.

Use CARGO_MANIFEST_DIR to build an absolute path to the workspace .git, so the watch logic is independent of process CWD.

Files reviewed: 33/34 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot AI lite review requested due to automatic review settings September 5, 2026 16:52

Copilot started reviewing on behalf of MikeGrier September 5, 2026 16:53 View session