Repository navigation
placement: plan_nodes checks its inputs and decides each node in helpers, and nic_not_max pins a window's one rounding - #163
Merged
juliankang4 merged 2 commits intoOct 14, 2026
Conversation
…ers of its own plan_nodes checked its inputs, built the candidate plans, decided each node and named the idle nodes in one function. The input checks move to checked_id_bytes, which returns one id's bytes over every routed layer, and the decision on a node to idle_why, which returns why the plan leaves the node idle or None where it takes it. Both are private; plan_nodes keeps the candidate plans, the loop over the nodes, the pool bound and the idle list. The checks keep their order and the decision its comparisons: a node is taken where the pools before it cannot hold every id and its own has room, and otherwise where the link stays within a tenth of the step and the step gains twice the ruler. The lines the move changes are the helpers' signatures, their calls and returns, and nodes, terms and used read through the plan (now.nodes, now.terms, used as a slice).
… in one rounding layer_ps divides a window's reads, rows and fills over one denominator, so the window rounds down once. No test turned red when the three terms each rounded down on their own: the link, gain and NIC cases hold every id, so no window carries a fill. nic_not_max gains a paged case on two nodes of one id's room each: forty ids of 649,500,000 B at top-k 1 read at 10^19 B/s, a hit rate of 999 permille and a miss's fill of 1,000 ps, split [20, 20]. Each window is a read of 64.95 ps and a fill of 19/20 ps, so the step is 325.7 ps and the predictor reads 324 ps. Node 0's NIC time is 161.75 ps, half of 323.5 ps: the line sits 2.2 ps under the step, more than the 2 ps a one-layer step can read low, so no node is named. Rounding the read and the fill apart reads each window 1 ps lower, a step of 323 ps, and names node 0.
7 tasks done
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Verdict: CLEAN: The extraction preserves validation and node selection, and the added case rejects separate window rounding.
Checked
- Duplicates: 2 new functions checked against existing placement owners; none duplicated. Dryer reported 0 pairs.
- Complexity:
idle_whyis new at 18; its decision branches moved unchanged fromplan_nodes. - Silent failure: 3 sites followed. Time conversions return named overflow errors; the idle fallback supplies a reason before filtering.
- Tests:
nic_not_maxprotects the combined division at crates/placement/src/placement/nodes.rs:1191. Separately rounding read and fill changes the derived step from 324 ps to 323 ps and returnsSome; the assertion requiresNone. - Gates: no pins, thresholds or cases removed or relaxed. The PR supplies mutation evidence for the added case.
- Adversarial checks: empty nodes and mismatched budgets fail before indexing; needed-node acceptance and ruler short-circuiting match the base.
- Performance: planning code only; all
plan_nodescallers are tests. - Escape catalog: no patterns matched.
Notes
Rust tests were not rerun. The PR reports 165 placement tests passing at each commit.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
Collaborator
Author
|
The review is done and the contribution check passed, so I'm merging this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
This follows up the two non-blocking points of the review on #95 (now in main as
33fb317b). It is two commits on mainbaeedd73, a move and a test, kept apart (R22):d51ec11fsplitsplan_nodes(R12). The input checks move tochecked_id_bytesand the decision on each node toidle_why. Behavior is unchanged.8d2ac015adds a paged case tonic_not_max. Before it, no test turned red when a window's read, rows and fill rounded down one by one instead of together.Code shape
The review reads these first. A box left unticked needs one line of why.
plan_nodes, moved once.nic_not_maxgains one case.cargo fmtandcargo clippyare clean on what I touched; the lint count did not go up (41, the ratchet).Duplication found or removed (
path:line), or "none": none.1. The split of
plan_nodes(R12)plan_nodeschecked its inputs, built the candidate plans, decided each node and named the idle nodes in one function of 131 lines. Now:checked_id_bytes(nodes, terms, budgets, workload)runs the checks in their old order (fabric, unified pools, budgets, the undefined inputs, overflow) and returns one id's bytes over every routed layer. The refusals stay listed onplan_nodes, the public function.idle_why(now, used, next, j)returns why the plan leaves nodejidle, orNonewhere it takes it. A node is taken where the pools of the nodes before it cannot hold every id and its own has room, and otherwise only where the link stays within a tenth of the step and the step gains twice the ruler. The comparisons are the same expressions.plan_nodeskeeps the candidate plans, the loop over the nodes with the pool check, the pool bound and the idle list, in 57 lines.Both helpers are private. Compared as a multiset of lines, the old
plan_nodesand the three functions differ in the helpers' signatures, calls and returns, inlet n = nodes.machine.nodes.len();, which bothplan_nodesandchecked_id_bytesnow read, and in three lines: the needed-node test readsnow.room(used)andnow.terms.experts, the ruler readsnow.nodes.ruler_permille, and the decision isOk(if ...)in place ofwhy[j] = if .... One path changes shape. A needed node used to be taken on the spot, withcontinue;idle_whynow returnsNonefor it, and the loop'sif why[j].is_none()takes it, as it takes a node that passes the link and gain lines. Either waywhy[j]staysNoneand the node joinsused. The placement tests pass 165 of 165 at both commits (155 at #95's merge; main has since added tenctxtests), and each mutant of #95's table that changesplan_nodesturns the same tests red as it did there (below).2. A window's one rounding
layer_pssums a window's read, its rows through the NIC and its fill over one denominator, so the window rounds down to whole picoseconds once. The mutant that rounds each term down on its own reads a window up to 1 ps lower where it has two of the terms, and 2 ps where it has all three [derived]. The link, gain and NIC cases all held every id, so no window had a fill, and the mutant stayed green.The new case pages, so each window carries a read and a fill, both with a fraction of a picosecond:
with_room), a hit rate of 999 ‰ and a miss's fill of 1,000 ps. The split is [20, 20], so each node pages 19 of its ids, a fill of 19/20 ps a slot.The line at 323.5 ps sits 2.2 ps under the step, more than the 2 ps the step can read low, so the predictor's verdict is the real one:
nic_bound()isNone. The mutant reads each window as 64 + 0 = 64 ps, a step of 323 ps, which is under the line, so it names node 0. The test asserts the split andnic_bound()only. The permille it reports moves about 1.5 ‰ for each picosecond of step here, and the contract does not fix it to that precision.What I ran
On a Mac (Apple M5) through
just, with mainbaeedd73as the scope base. The placement crate is pure and runs natively.just mac-testran at each commit; its summary line, and the placement library's result:The one failure at both commits is
bloomery-refset'sarch::draft::tests::a_plain_target_with_no_draft_beside_is_the_family_path, which compares a/var/folderstemporary path with its/private/var/foldersform. It fails the same way on main, and this PR does not touch that crate.just mac-static upstream/mainat8d2ac015(upstream/main =baeedd73):The move needs no
ptx-scanor timed A/B: the placement crate links no kernel, nothing outside it calls the node plans yet, and the planner runs when a plan is made, not in the step.The mutants ran one at a time on this branch's tree before the rebase onto
baeedd73(then on33fb317b;nodes.rsandnodes_scale.rsare byte for byte the files at8d2ac015, and main changed no other file those tests read), each followed bycargo test -p bloomery-placement --lib nodes_scale. They are #95's table with the window row now red. Six of them changed a line that moved into a helper, so their text follows the line: the ruler fixed at 20 and at 10 ‰, the link share and the gain read in whole nanoseconds, the link share read on the plan without the node, and the two rules for a needed node. Each makes the same change as before.Every mutant built. The rows for
plan_nodes, the move's evidence:for j in 1..n.min(2))fills_balance,link_share,messages_o_n,never_slower,nic_not_max,pool_per_node2 * 20u128)never_slower2 * 10u128)never_slower/ 1000 * 1000on both steps and the link inidle_why)link_share,never_slowerif !now.over()?.is_empty())fills_balance,nic_not_max,pool_per_node,read_balancenic_not_max(the new case)&& now.room_ids(j) > 0)pool_per_nodefixed > poolcheck inplan_nodes)pool_per_nodepool_per_nodeprompt_rate == 0refusal inchecked_id_bytes)named_refusalsnow.exposed_ps(a)and> afornext's andb)link_shareBesides the old rule for a needed node above, two rows turn more tests red, each through the new case:
layer_ps)nic_not_max+ POST_NS * 1000on node 0's base)fills_balance,link_share,n1_is_today,never_slower,nic_not_max,read_balancenic_not_maxThe other 26 rows match #95's table. Of these, 25 turn the same tests red, and one stays green, as on #95: the optimum's share sum in u64, which cannot pass u64 in a test, since each share is at most 1000 × experts and the split walks every id.
Hardware (if the change is for a machine)
None. Every number above is the predictor's [derived]; nothing was timed. The Mac checks ran on an Apple M5 (32 GB) with
rustc 1.100.0-nightly (e457a7b0d 2026-08-27).AI help
AI 코딩 에이전트가 코드를 작성한 후 설명을 썼고, 테스트를 마쳤습니다. diff 리뷰와 PR 초안 리뷰는 제가 직접 했습니다.