Skip to content

Give each schedule worker its own claim variable - #73

Merged
asinghvi17 merged 1 commit into
claude/perf-ladderfrom
claude/guided-schedule-race
Aug 23, 2026
Merged

Give each schedule worker its own claim variable#73
asinghvi17 merged 1 commit into
claude/perf-ladderfrom
claude/guided-schedule-race

Conversation

@asinghvi17

Copy link
Copy Markdown
Member

GuidedSchedule taper has been failing intermittently in CI and blocking the
G-stack: PR #70 failed 1.12 - macos-latest and passed ubuntu, PR #71 failed
1.12 - ubuntu-latest and passed macos, same assertion at
test/scripts/copdem_policy.jl:107, and neither PR touched the scheduler.

It is the test, not claim!

The tail-loop directly above already binds b in the testset scope:

while (b = claim!(s)) !== nothing
    last = b
end

so the b in the concurrent loop below it is not a fresh local — it is that
same binding. Eight Threads.@spawn closures assign to it, so lowering hoists
it into a single Core.Box that all eight capture. Confirmed both ways:
Meta.lower emits Core.Box for b only when that tail loop is present, and
at runtime four tasks writing four distinct values all read back one value.

The optimized closure shows what that costs:

setfield!(box, :contents, %2)     # my claim
%5 = (%2 === nothing)             # the loop test reads the fresh SSA value
...                               # seen[w], bounds-checked
%32 = getfield(box, :contents)    # append! RE-READS the shared box
append!(%26, %32)

Between the store and the re-read, any of the other seven workers can store its
own batch. The loser appends the winner's range: one batch counted twice, its
own dropped. Length stays 5000 and both ends stay clean — precisely the shape
CI printed, with the damage in the elided middle. A worker storing nothing on
its way out can also hand a live worker append!(seen[w], nothing).

Reproducer

The store-to-load window is a few instructions, which is why this never fired
on the dev box (five clean local runs, 8000 trials at 2/4/8/16 threads, 5M
claims at n = 2_000_000: nothing). Widening it with --compile=min fires it
readily:

shape threads trials mismatches task exceptions
as committed 8 200 13–16 32–58
as committed 4 200 15 6
this PR 2 / 4 / 8 / 16 2200 0 0

Every mismatch was count = 5000, duplicated = 8, uncovered = 8 — one batch
doubled, one batch dropped.

claim! is correct, and production was never exposed

claim! is unchanged. Its CAS loop was stressed at 32.4M claimed positions
over a grid of n (0, 1, 2, 7, 8, 9, 63, 64, 1000, 100 000), workers
(1, 2, 8, 24) and maxbatch (1, 3, 8, 64), at 8 and 24 threads with tasks
oversubscribed against workers: every run covered 1:n exactly once and left
the cursor at n + 1.

Production runchunks in scripts/copdem_production.jl lowers with zero
Core.Box — its batch is bound only inside the spawned closure, and the
closure captures only read-only state (chunks, order, sched, dem, srcspace, sys7, store, layout, prefetcher, config, log, p, w). The completed
66,228-column run's "0 skipped, complete store" stands; no column was double-
regridded or dropped by this. The only other claim! loops are single-threaded.

The fix

Hoist the claim loop into drain!, so b is a local of one call and the spawn
body assigns nothing. Only s remains boxed, and it is read-only while the
tasks run.

The assertion is also split into named parts — count, duplicated,
uncovered, stray — so the next miscover says what was wrong instead of
printing two collections that are identical at both ends.

Not on the stack

Cut from origin/claude/perf-ladder, independent of #70#72, so it can merge
without waiting on them.

`GuidedSchedule taper` failed intermittently in CI — PR #70 on
1.12-macos, PR #71 on 1.12-ubuntu, the same assertion, neither PR
anywhere near the scheduler. The cursor was not the problem.

The testset's tail-loop above already binds `b` in the testset scope:

    while (b = claim!(s)) !== nothing
        last = b
    end

so the `b` in the concurrent loop below it is not a fresh local. It is
that same binding, and because eight `Threads.@spawn` closures assign to
it, lowering hoists it into one shared `Core.Box` they all capture. The
optimized closure is explicit about the consequence:

    setfield!(box, :contents, %2)     # my claim
    %5 = (%2 === nothing)             # loop test reads the fresh value
    ...                               # seen[w], bounds-checked
    %32 = getfield(box, :contents)    # append! re-reads the SHARED box
    append!(%26, %32)

Between the store and the re-read, any of the other seven workers can
store its own batch. The loser then appends the winner's range: one
batch counted twice, its own dropped. The total stays 5000 and the ends
stay clean, which is exactly the failure CI printed — the damage is in
the elided middle. A worker that stores `nothing` on its way out can
also hand a live worker `append!(seen[w], nothing)`.

Reproduced by widening that store-to-load window with `--compile=min`:
13-16 mismatches per 200 trials, every one of them `ndup = ngap = 8`,
plus task exceptions from the `nothing` case. 2200 trials of the fixed
shape across 2, 4, 8 and 16 threads: zero.

Hoisting the loop into `drain!` makes `b` a local of one call, so each
task owns its own; the spawn body no longer assigns anything.

`claim!` itself is correct and unchanged. Its CAS loop was stressed at
32.4M claimed positions over a grid of n, workers and maxbatch at 8 and
24 threads with tasks oversubscribed against workers: every run covered
`1:n` exactly once. Production `runchunks` lowers with zero `Core.Box` —
its `batch` is bound only inside the spawned closure — so the completed
66,228-column run was never exposed to this.

Report the miscover in parts, too, so the next one names itself:
count, duplicated, uncovered, stray, rather than two collections that
print identically at both ends.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019DuGKTmvs5B4EynwZddKMg
@asinghvi17

Copy link
Copy Markdown
Member Author

The failure inventory is worse than it looked: 4 occurrences, not 2

I went back to the raw job logs. The Julia 1.11 - ubuntu-latest failures on
#70 and #71 are this same bug, not the pre-existing allocation-law asserts
they were assumed to be:

PR 1.11-ubuntu 1.12-ubuntu 1.12-macos
#70 FAIL copdem_policy.jl:107 pass FAIL copdem_policy.jl:107
#71 FAIL copdem_policy.jl:107 FAIL copdem_policy.jl:107 pass

Both 1.11 jobs report 1044015 passed, 1 failed, 0 errored, 58 broken, and the
one failure is GuidedSchedule taper at line 107 with the same
[1, 2, … 4999, 5000] == 1:5000 print. There is no second failure in either log.

The "1.11-ubuntu is red on main for 8 allocation-law asserts" belief is stale —
#61 (Assert the allocation laws only on Julia 1.12, merged) turned those into
broken, which is why 1.11 shows 58 broken against 1.12's 45. 1.11-ubuntu has
no other failure.

So this one test was the only thing red on #70 and #71, on every job that was
red. Fixing it should take both PRs fully green.

@asinghvi17
asinghvi17 merged commit b6a08ae into claude/perf-ladder Aug 23, 2026
6 checks passed
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.

1 participant