Stop keeping a second framebuffer, and a second count grid - #322
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Two Rust-core allocations that existed only to be copied out of.
`Canvas` owned its pixels: `rasterize_spans_into` allocated `w * h * 4`, zeroed
it, painted it, then `copy_from_slice`'d the result into the caller's
identically shaped output. Two full framebuffers were live for the whole paint —
4 bytes per device pixel of pure surplus, 33 MB at 3840x2160 — plus an
allocation, a zero-fill and a memcpy. `Canvas` now borrows its framebuffer, so
the RGBA entry points paint straight into the caller's buffer and the export
step is nothing at all. Measured on a 3840x2160 RGBA raster in a fresh process:
peak RSS 73.3 MB, growth 33.4 MB, against 104.9 MB and 64.8 MB before — exactly
one framebuffer's difference. `rasterize_png_spans_into` still owns a buffer
because its destination is the encoded stream, but that one is 3-channel.
Failure semantics are unchanged in substance: a malformed display list already
left the output undefined and every Python entry point raises rather than
returning it, which the docstrings say ("the Rust side returns 0 = output
undefined").
`bin_2d_counts` allocated a fresh `w * h` u32 grid to merge its worker grids
into, and then overwrote every cell of it. It is the only member of the bin_2d
family that does not merge into a caller-owned buffer, and it is the one on the
pyramid build path, so peak was (threads + 1) grids where threads was enough:
16.8 MB of surplus on a 2048^2 base and considerably more out-of-core, since an
adaptive base_dim keeps n/cells in roughly [4, 16] and therefore keeps the
parallel branch live at much larger grids. Fold into the last worker's grid
instead. Integer addition is associative and the single saturation point is
unchanged, so the output is bitwise identical for any thread count.
118 Rust tests and 2412 Python tests pass; a 97-artifact output fingerprint
sweep is identical to main.
3d840ff to
51f37e3
Compare
1 participant
Two Rust-core allocations that existed only to be copied out of. From the second
memory audit pass (follow-up to #309); sibling PRs carry the encode paths (#320),
the Python paths (#321), the selection kernel, and the legend cache.
The rasterizer kept a second framebuffer
Canvasowned its pixels.rasterize_spans_intoallocatedw * h * 4, zeroedit, painted it, and then
copy_from_slice'd the result into the caller'sidentically shaped output. Two complete framebuffers were live for the whole
paint — 4 bytes per device pixel of pure surplus, 33 MB at 3840x2160 — plus an
allocation, a zero-fill and a memcpy nobody asked for.
Canvasnow borrows its framebuffer (px: &'a mut [u8]), so the RGBA entrypoints paint straight into the caller's buffer and the export step is nothing at
all.
Canvas::wrapzeroes the destination exactly as the owningvec![0; w * h * 4]did, and straight alpha is already the canvas's own layout,so
to_rgba8had no work left to do and is gone.paint_banded(
split_at_mut) andpaint_image_bands(chunks_mut) already operated on&mut [u8]and needed no change.Measured on a 3840x2160 RGBA raster in a fresh process:
Exactly one framebuffer's difference.
rasterize_png_spans_intostill owns a buffer, because its destination is theencoded stream rather than a caller frame — but that one is 3-channel.
Failure semantics are unchanged in substance. A malformed display list already
left the output undefined, and every Python entry point raises rather than
returning it —
_native.rasterize's docstring says so explicitly ("the Rust sidereturns 0 = output undefined"). The difference is only that the undefined bytes
now live in the caller's buffer instead of a discarded one.
bin_2d_countsallocated a merge target it then overwroteAfter collecting
threadsfull worker grids, the parallel branch allocatedanother full
w * hu32 grid and summed into it — so peak was(threads + 1)grids wherethreadswas enough. It is the only member of thebin_2dfamily that does not merge into a caller-owned buffer, and it is the oneon the pyramid build path, i.e. the largest grid in the codebase.
That is 16.8 MB of surplus on a 2048^2 base, and considerably more out-of-core:
_pyramid_base_dim_forroundssqrt(n / 16)up to a power of two, which keepsn / cellsin roughly [4, 16] and therefore keeps the parallel branch live atmuch larger grids (a memmapped 500M-row trace builds at base_dim 8192, where the
surplus is 268 MB).
Fold into the last worker's grid instead. Integer addition is associative and the
single saturation point is unchanged, so the output is bitwise identical for any
thread count — the invariant the doc comment and
bin_2d_threads_grid_awarecare about.
Verification
Canvasborrows now, so tests that keep a result past thecall use a new
rasterize_to_vechelper that owns the buffer; the comparisonsthemselves are unchanged.
exports, selections, density views, append, pyplot — is identical to main.
Perf
This is the tranche with real perf exposure, so it was gated on its own. Focused
interleaved A/B against main over the raster and binning regions, 16 alternations
in drift-cancelling ABBA blocks, native core swapped per arm:
A first full-suite run read
pyramid_build+14.9%, which did not survivescrutiny: the control kernels (
normalize_f32,min_max,range_indices— codethis PR cannot reach) moved by the same amount, and
pyramid_buildshowed 54-74%intra-arm spread with monotonic drift across the run. Re-gated on a quieter
machine with balanced round ordering it is -7.7%.