RunTasks keeps its RunState - counter, mutex, condition variable - on its own
stack frame and hands a pointer to the pool. The last worker to finish
decremented the counter OUTSIDE the mutex and only then took it to notify, while
the waiter's predicate was the counter itself. So the waiter could see zero the
instant the decrement landed, find its predicate already true, never block, and
return from RunTasks - popping the frame. The worker then locked a mutex and
signalled a condition variable that no longer existed, writing pthread state into
a frame the submitting thread had already reused.
pthread_mutex_unlock writes owner and nusers as eight contiguous zero bytes. Land
those on a live pointer and the next read of a member at offset 8 faults: the
observed crash was fmt's buffer<char>::append with this == nullptr, in the log
call immediately after a parallel pass, which is why it always appeared after the
resolution-filter line - that line was simply the next thing to use the frame.
Give the waiter a flag set under the same lock as the notify. Completion cannot
then be observed until the notifier has released the mutex, i.e. after its last
touch of the state. The counter keeps its lock-free fast path and decides only
who notifies, so there is still exactly one lock per pass.
Found independently by two investigations: a widened-window reproducer (2 crashes
in 38 unfixed, 0 in 60 fixed; glibc's own "__owner == 0" assertion caught in the
pool worker) and an isolated one that clobbered a freshly filled stack frame 732
times in 60000 and never after the fix. Growing RunState by eight bytes, changing
nothing else, took the rate from 0/90 to 3 hard failures in 30.
The dataset that failed about one run in twelve: 0 of 40. Space group and
merged output unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ParallelChunks and ParallelFor started a thread per chunk with std::async and joined it
again on every call. A thread costs tens of microseconds to create and join, and the
analysis code repeats some of these passes thousands of times in a run - the merge alone
has dozens of call sites, several of them inside iteration loops - so a short pass could
spend more on its threads than on the work.
Both now run on a pool made once and kept. The split is unchanged, so a pass whose
per-element work is independent still gives the serial answer bit for bit.
Two properties the futures gave for free had to be kept explicitly. A pass reached from
inside a pool worker runs inline instead of queueing, since the workers are occupied by
the outer pass and waiting for one of them could wait forever. And an exception from any
task is held until every task has finished and then rethrown to the caller, so the others
still run to completion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two thirds of a rotation run is one thread. The image loop is not the problem -
on the heaviest crystal of the battery it is 1.8 s of 40 - and neither GPU nor
CPU is saturated, because while the corrections and the space-group search run
there is one core working and 47 idle. Mean occupancy over the whole run: 3.9 of
48.
In the correction surfaces (absorption in the goniometer frame, detector-plane
modulation, absorption against time and detector position - all one function):
the per-cell accumulation, the score reduction and the final apply are now
chunked, as are the three loops that assign a full to its cell, one of which
spends a sine and a cosine per full de-rotating it into the crystal frame. Two
full sorts of four million floats went with them: only the nine bin edges are
wanted, so they are selected instead, each selection starting where the last one
left off.
The per-group pass is deliberately left serial. The terms of one group are
spread all over the list, so the only way to give a thread groups of its own is
to walk in group order, and that trades a near-sequential read of the fulls for
a random one over a few hundred megabytes - the trade that already lost once in
the combine kernel.
The space-group search scores each candidate rotation by correlating I(h)
against I(Rh) over the whole merge. Every operator it can ask about comes from a
fixed list and none of them depend on each other, so they are scored up front,
in parallel, and the search reads the cache. The scratch that stops a pair being
counted twice is now per worker rather than shared.
Worker counts are gated on how much work there is, not on how many cores the
machine has (ThreadsForWork). Both parallel helpers start a thread per chunk, so
a small dataset on a large node would otherwise pay for 48 thread starts to sum
a few thousand terms - and this runs on 8-core laptops as well as on this node.
Measured on the heaviest crystal, idle machine, two runs each, summed over both
passes: those phases go 7.88 s -> 5.19 s. Whole-run wall time is the wrong ruler
for it - it moves +-4 s between identical runs. Battery 9m45s -> 9m23s, space
group 21/24, no failures; 16 of 24 crystals bit-identical to the previous run
and the rest inside the noise floor of running one binary twice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two shapes - a fixed contiguous split, and work stealing off an atomic - had been copied into
whichever file wanted them: an anonymous namespace in RotationScaleMerge.cpp, and another, byte for
byte the same, in ShadowFinder.cpp, whose comment claimed it was "the only other file that wants
it". The header is taken from the beam-stop GPU commit on the performance branch, which is not
otherwise being picked.
ShadowFinder now uses it, so the construct is exercised rather than shipped unused, and its own copy
is gone. The beam-stop mask is unchanged, which its reference count already pins.
Tested for the properties the callers rely on and which are easy to lose in a rewrite: the chunked
slices tile the range in order with no empty one, work stealing visits every item exactly once, one
thread means the caller's loop in order, an empty or negative count does nothing, an exception in a
worker reaches the caller, and - the point of the whole thing - the answer is the serial answer bit
for bit at every thread count.
RotationScaleMerge.cpp keeps its own copy for now; consolidating it belongs with the scaling work,
which is not being touched here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VfYvJT5Nb71suJCowRBn5z
leonarski_fandClaude Opus 5