perf: reciprocal-space mapping locality, grouping and per-call scheduling - #76
Merged
Conversation
Golden digests fail across compilers, so the adaptive rule is reimplemented in Python and compared record for record at depths 0-4.
The dyadic corner cache switched off above depth 3, so the very_high and maximum presets recomputed every corner, at 74 CPU-hours for one real job. Output is bit-identical and 1.65x to 2.98x faster. Profile counter coordinate_evaluations changes meaning.
Adjacent frames sit as close in reciprocal space as adjacent pixels, so a (row, column, frame) brick measures 42% fewer records. Conditional on per-frame displacement staying near one voxel; not implemented.
Both memory prechecks sized a pixel at its worst-case leaf count, claiming 32.7 GB for a Pilatus-6M frame at depth 3. That rejected any useful detector tile and collapsed image_workers to one. Changes how many frames run concurrently, not any record value.
What the record-based bound unblocked, and why it resolved the Python/kernel drift by removing the exposure dependence from both sides.
Consecutive frames land as close in reciprocal space as adjacent pixels, so a (row, column, frame) brick emits 0.575x the records. Not bit-for-bit, though every voxel and contributor count is unchanged. accumulate() is untouched and remains the per-image path.
Measured record and time ratios against the geometric prediction, plus the correction that a single-frame group is not bit-for-bit.
Compares group bricks against the same frames mapped one at a time, reporting records before the cross-block merge and wall time.
The pipeline now calls accumulate_group, making a work block a brick in (row, column, frame) rather than a run of one image. Records reaching the checkpoint router fall to 0.598x at eight frames per group. BREAKING CHANGE: reconstructed values move in the last bits for every job, including those mapping one frame at a time, since contributions now merge inside the kernel rather than in the checkpoint accumulator. Which voxels are reached and how many samples reach each are unchanged. Checkpoints stay resumable, but a job resumed across this change holds parts from both association orders.
Times the whole mapping phase rather than one kernel call, which is what grouping has to actually improve. It reports records entering the checkpoint layer, not rows on disk, where the saving never shows.
The record prediction held at 0.598x; the throughput one did not, since image_workers collapses when a worker is charged for its resident frames. The scheduler must come before per-job group sizing.
Grouping was not worth turning on while the per-frame scheduler charged a worker for every resident frame. Moving correction into a prepare pool takes mapping to 0.866x the per-frame pipeline at four frames per call. A different number of concurrent calls changes how sums associate.
The knobs the grouped scheduler turned out to need measuring against; the final progress line is now reported alongside the timing.
Mapping is 0.866x the per-frame pipeline at four frames per group. Corrects the thread-saturation claim, and records that this machine varies 30% run to run, so arms must be interleaved.
frames_per_group now defaults to measuring the job, picking four frames per call at 0.88x mapping time. Chosen on throughput, since density alone picks the slowest configuration. Turns grouping on by default, so contributions associate differently.
--group 1 is now how the per-frame arm is requested explicitly.
The measured choice reproduces step 5's hand sweep: four frames per call at 0.88x the per-frame pipeline. Band height is still taken as given.
Tile planning was the third site still sizing from worst-case leaves, collapsing a band to one row at depth 5: 2527 native calls per image instead of six. Throughput is a wash, so this is for correctness.
Swept at four frames per call the surface is flat, so the pair needs no joint search -- the default already sits in the region that matters.
Two design documents were referenced by the tracked locality document while untracked themselves, so a fresh clone got dangling references. All three now agree with what has landed.
--depth replaced max_depth alone, leaving work_block_pixels at whatever the job's own accuracy implied. The preset halves with depth to hold the native working set fixed, so a depth-0 arm ran an eight-times-too-small block -- and frame-group brick dimensions are cut from that same count, which biased the one comparison this benchmark exists to make.
Record counts converging with depth says nothing about whether an intensity moves. This compares weighted_intensity / weight across depths against each voxel's own propagated error bar, and reports the exposure at which a difference would become visible -- which is the answer independent of how long the probe could afford to run. Unlike the other reconstruction benchmarks it compares numbers rather than times, so it needs no interleaving, repeats or a quiet machine.
Frame grouping's record saving transfers above depth 0 and even improves there, but it stops converting into time: depth 1 and depth 2 are nulls on both grids, while the depth-0 control reproduces phase 3 at 0.872x with every pair favouring grouping. Above depth 0 each pixel and frame still walks its own subdivision tree, so record density no longer governs kernel time and no brick shape can recover it. Pixel-outer iteration and re-derived brick dimensions are retired unbuilt. Records the prior question with it, which turned out to be the larger result: depth 3 differs from depth 5 by 0.013-0.024 of a voxel's own error bar, so balanced accuracy is enough for a full job and the three settings above it are not distinguishable from it by the data. Also records that the reference job on disk no longer has the grid every phase 1-3 number was taken against, which invalidated a whole sweep before it was noticed.
Frame grouping pays only where the kernel dominates and record density dominates the kernel, and nothing above balanced accuracy changes an intensity measurably. The traps are the ones that cost time this round. The reference job's own grid has changed since these numbers were taken. The page cache, not thermal state, dominates depth-0 noise, because the window is 11.7 GB against 31 GB of RAM. Foreign load must be summed per process: taking it as machine-busy minus the benchmark charges the benchmark's own I/O kernel time to something else, reporting 7.2 cores of interference where per-process accounting showed 2.9. And a run that maps nothing looks like a fast run, so a checkpoint fingerprint has to be compared against its partner's.
The footprint accuracy settings were described by what they do, not by what they are worth. Each one's effect on a reconstructed intensity is now given against that voxel's own counting error, together with the exposure needed to see it. Balanced is sufficient for a full rotation scan, which is why it is the default. High, Very high and Maximum shift an intensity by 0.02 sigma or less and are not distinguishable from Balanced by the data, at roughly 4x, 15x and 100x the runtime. Center only is genuinely lossy and also misses about 4% of the voxels the other settings reach. No behaviour or default changes.
Roughly one run in twenty mapped every frame, routed nothing, wrote a checkpoint claiming its full frame count with zero rows, and exited successfully in a fifth of the usual time. Nothing downstream could tell that from a fast but legitimately empty run, and on resume the empty part counts as done. This warns rather than raises, because a grid can genuinely cover a region a given slice of the scan never reaches. It reports through the progress channel as well as warnings, since a bare warning is invisible in the GUI, where an empty reconstruction is most expensive to discover late. Both schedulers are covered: the grouped one returns early, so a check placed only at the end of the function would miss every grouped job, which is the default whenever a scan's geometry qualifies. The root cause is not yet known, and this does not address it. The static correction factor, the exposure angle bounds and the grid are all bit-identical across fresh job loads; both schedulers re-raise recorded exceptions; and the native memory precheck throws rather than returning empty.
Four candidates measured and ruled out rather than reasoned about, so that the next person does not spend the time again: the static correction factor, the exposure angle bounds and the grid, a swallowed exception, and the native memory precheck. Thirty consecutive runs of one window returned the identical record count, so it is not a plain per-run race either. What remains is a raw frame reading back non-finite for a whole run, which the finite-value check then masks away entirely. Unproven.
No tag contains the reciprocal-space reconstruction feature, so the association change has no published behaviour to break and no stored result a user could have produced under the old order. Recorded as decided rather than left pending. The changelog had described this as changing reconstructed values for every job, which says nothing to a reader for whom the entire feature is new. What survives is the part that is still true: a job resumed against a scratch directory written by an earlier development build will hold checkpoint parts from both association orders.
Phase 4 measured something it was not looking for: at depth 0 the pipeline runs at ~110 ms/frame using 7-8 of 24 cores, bound by GIL-held loading and correction rather than by mapping. That is the largest remaining win in the mapping phase, and it takes over findings open item 1 as a plan of its own. Three mechanisms in order, each with a predicted band and each measured on its own: drop the per-tile stack and copy, fuse and GIL-release the correction, then extend the prepare pool to the per-frame path. The first step is none of them but a re-measurement -- the existing stage table predates grouping and was taken in a kernel-dominated regime, so it cannot apportion 110 ms/frame today. Corrects the headroom claim in passing. Cold reads cap the pipeline at 51.5 ms/frame, so the prize is 2.1x; an earlier draft reasoned from idle cores alone and said 3x. Also records what phase 3 step 2 actually shipped: the per-brick centre ray precompute is implemented and was never marked done, while the whole-detector centre-ray cache it suggested is not, and is estimated at 2-4% for 149 MB.
The stage benchmark splits the mapping phase by stage and reports wall against on-core time for each, from QueryThreadCycleTime rather than time.thread_time, which quantises to the 15.6 ms scheduler tick. It also probes GIL contention directly. The A/B driver runs two arms alternating, flipping their order each repeat, and reports paired ratios, per-run foreign CPU load and each run's checkpoint fingerprint. An arm is a set of files copied into the checkout, so a native change measures against its own baseline binary.
Detector tiles partition the detector, so gathering each tile into its own (frames, rows, columns) buffer copied every corrected frame exactly once per group -- ~105 MB a frame, in Python, on the compute worker -- purely to give accumulate_group a shape it did not need. accumulate_group_tile takes the whole frames plus the tile rectangle and reads it through the frame's own row stride. Both entry points build the same GroupPixels view and run the identical brick loop, so this is bit-for-bit: measured 0.840x end to end at depth 0 over six interleaved pairs, with identical voxels, contributors and totals.
The per-pixel static factor, each scalar factor in turn and the finiteness check that masks non-finite pixels were eight or nine full-detector NumPy passes over ~50 MB each, taking and dropping the GIL between them. They are now one native pass, GIL released throughout. Bit-for-bit, not merely equivalent: correction is entirely element-wise, so there is nothing to reassociate. The caller passes the squared factor it already computed rather than letting the kernel recompute it, since x ** 2 is pow in Python and a multiplication in C++, and the propagated branch keeps its order -- scale the variance, use the intensity from before the factor, then scale the intensity. A test pins both branches. Measured 0.923x end to end at depth 0, against an independently measured ceiling of 0.928x for removing the arithmetic entirely. The saving is memory traffic, not GIL: the pool that runs correction is idle 87% of the time either way.
The premise was wrong. A GIL probe measures 23% contention at depth 0 and the prepare pool is 13% occupied, so correction is already overlapped and nothing is serialised behind it. What holds the pipeline at 6.4 of 24 cores is memory: the same correction costs 2.3x the cycles in-pipeline that it costs alone on one thread. So work removed anywhere pays, including off the critical path, and moving work between threads pays only where it was queued. A (0.840x) and B (0.923x) are both traffic reductions and both landed; C was scheduling only, measured 1.035x on the arm it was written for, and is reverted. Two of four predicted bands were missed and are recorded as such. Also records three things the sweeps caught: a zero-record run on an unchanged arm at 46.4 ms/frame rather than the documented fifth-of-normal speed, a run that mapped identical voxels and contributors while carrying 22% more total intensity, and the pipeline hang caught alive with every checkpoint written and 58 threads at zero CPU. Includes the uncommitted zero-record investigation write-up that was already in the working tree.
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.
perf: reciprocal-space mapping locality, grouping and per-call scheduling
Summary
This branch is a multi-phase performance pass on reciprocal-space mapping (
orgui/reconstruction_job.py,orgui/datautils/xrayutils/reconstruction.py, the C++ kernel), driven throughout by measurement against a real 3651-frame Pilatus 6M rotation-scan job rather than synthetic benchmarks. No mapping convention, voxel selection, or default behaviour changes; every record-shape and value change below is verified bit-for-bit or pinned against an independent reference test.Design records and measurements are under
doc/design/(not published, not in the Sphinx toctree — seedoc/AGENTS.mdfor how these are meant to be read and maintained going forward).What changed, and why
89f969c), pinned against an independent reference test (415dc1f) before anything downstream touched it.physchange even though voxel selection and per-voxel sample counts are unchanged.very_high/maximumaccuracy — one row per band on a Pilatus 6M atmaximum, 2527 native calls per image instead of six.balancedthroughput is unchanged; this only mattered at the two highest accuracy settings.Center onlyaccuracy, bit-for-bit identical output.Center only, biggest where mapping itself is cheap (memory-traffic saving, not compute).Balancedis sufficient for a full rotation scan against each voxel's own counting error;High/Very high/Maximumshift intensities by ≤0.02 sigma (indistinguishable fromBalanced) for 4x/15x/100x the runtime.Center onlyis lossy and misses ~4% of reachable voxels. No default changed.doc/design/reciprocal_space_mapping_serial_fraction.md): the premise — that the pipeline is GIL-serialization bound — measured false. The machine is memory-bandwidth bound, not core bound, at low adaptive depth. Two of the three candidate fixes under that hypothesis are the tile-copy and image-correction changes above; the third (a pure scheduling change, no traffic reduction) was implemented, measured as a regression, and reverted. That negative result is recorded rather than discarded.Net effect
Compounding the changes that survived measurement (grouping, scheduling, tile read-in-place, fused correction) gets to roughly 0.77x wall time at depth 0 on the reference job relative to branch start, with unchanged voxel selection and bit-for-bit-identical results wherever the change isn't explicitly a
physreassociation (frame grouping).Validation
pytest orgui/datautils/xrayutils/test/test_reconstruction*.py(includes newtest_reconstruction_group.py,test_reconstruction_pipeline.py,test_reconstruction_subdivision.py, and expandedtest_reconstruction_job.py/test_reconstruction.py)benchmarks/benchmark_reconstruction_*.pyscripts added to reproduce every measurement above against the real job, not synthetic dataCHANGELOG.md/doc/source/release_notes.rstupdated with full, measurement-backed entries for each user-visible changeCompatibility notes