perf(image): hold inflated pixels softly so the heap can reclaim them - #2027
Merged
Conversation
VCImageCompressed kept its inflated pixels in a strong transient field, so once an image had been read its 62 MB stayed resident for as long as the image object did -- even though the compressed form it can be rebuilt from is about 1 MB. Real geometry images compress 50-100x (measured across 62 images in the VCML corpus: median 73.6x, 63.5x for those over 1 MP). The inflated array now hangs off a SoftReference and getPixels() re-inflates on demand. Demonstrated under the shape of the #2021 incident -- 11 images all reachable at once, 256 MB heap: soft cache OFF OUT OF MEMORY after 9 of 11 images soft cache ON completed all 11; heap drops 246 MB -> 63 MB at image 10 as the collector reclaims, then 283 MB of pixels are re-read on demand SoftPixelCacheDemo reproduces both sides. It is a main, not a test: driving a JVM to the edge of its heap does not belong in a shared test run. SOFT, not weak, and the difference is the whole design. A WeakReference is cleared at the next GC whatever the heap looks like, so an image in active use would re-inflate on essentially every collection -- ~90 ms of CPU repeatedly, for nothing. Soft references are cleared only under real pressure. Nothing about ordinary behaviour would look wrong if someone swapped the type, so ordinaryGcDoesNotDiscardThePixels exists to fail if they do: verified by temporarily switching to WeakReference, where it fails with a changed identity hash while the rest still pass. That control also improved the tests. With WeakReference in place the reflection helper threw ClassCastException and turned two unrelated tests into errors, burying the one failure that actually explains the problem. The helper is now typed as Reference<?>. getPixels() takes a strong local reference once and holds it for the whole method. Reading the SoftReference twice would let the collector clear it between the null check and the return, handing the caller a null array -- a race that would be rare, non-deterministic and extremely unpleasant to diagnose. Two loops hoisted, because they call getPixels() every iteration and can receive a COMPRESSED image: ClientRequestManager:1169 (reached as getGeometrySpec().getImage(), the stored image) and ImageFile:149/153. Under the very pressure that clears a soft reference, those would have re-inflated per iteration. The other two loops the audit found are deliberately NOT touched -- ROIMultiPaintManager:2036 and ITextWriter:544 operate on locally built VCImageUncompressed instances, which hold their pixels in a final field and never inflate. Escape hatch: vcell.image.softPixelCache=false restores strong caching. Depends on the parent commit. Softening this cache does nothing on its own while GeometrySpec holds a second strong reference to the same array. vcell-core Fast: 587 run, 0 failures, 8 errors -- the MathOverrideRoundTripTest and VCellDataTest errors docs/BUILDING.md documents for a worktree without the Poetry environments. Refs #2021, #2025 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
jcschaff
added a commit
that referenced
this pull request
Aug 24, 2026
The compressed-and-rehydrate direction is built: #2026 removes the second strong reference GeometrySpec held to the image's pixel array, #2027 makes VCImageCompressed hold its inflated pixels through a SoftReference. Both are open, neither is merged, and the sequencing question in section 6 is still open. Adds section 3.9, the end-to-end check that section 3.7's arithmetic actually changes the outcome. Eleven images, 300^3 each, 256 MB heap: soft cache OFF OUT OF MEMORY after 9 of 11 images soft cache ON completed all 11 With it on the heap drops 246 MB -> 63 MB at image 10 as the collector reclaims, and 283 MB of pixels are re-read on demand. That is the incident and its fix in one run. Rewrites 5.3b as built rather than proposed, and is explicit about what it did NOT cover, because the heading alone would overstate it: - VCImageUncompressed still has no compressed form, so the SAMPLED image is still held strongly. Derived rather than stored, so a different problem. - The per-pixel path is unchanged. The loops that could see a compressed image were hoisted; VCImage.getPixel(x,y,z) and ImageSubVolume.isInside still fetch per call. Cold today, a problem only if either becomes hot. - The label array is untouched -- that is 5.3, and still the largest single retained object. Also corrects two places in section 3.8 that still read as open, updates the branch table with a state column now that drafts and open PRs are mixed, and expands the reproduction commands to include SoftPixelCacheDemo (which needs its own -Xmx, so it does not go through exec:java). Refs #2021, #2026, #2027 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
jcschaff
changed the base branch from
fix/geometryspec-pixel-retention
to
master
August 24, 2026 02:55
jcschaff
added a commit
that referenced
this pull request
Aug 24, 2026
#2026 (8a6e9f7) and #2027 (620ccd5) landed on master 2026-08-24. Admin-merged because master requires a review and none was available, which bypasses the merge queue, so regression.yml was triggered manually against master. Everything else in this document is still open: #2022, #2023 and #2024 remain drafts and none of the decisions in section 6 have been made. Refs #2021, #2026, #2027 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
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
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.
Implements §5.3b of #2025. Stacked on #2026 — the diff below is this commit only; softening this
cache does nothing on its own while
GeometrySpecholds a second strong reference to the same array.VCImageCompressedkept its inflated pixels in a strongtransientfield, so once an image had beenread its 62 MB stayed resident for as long as the image object did — even though the compressed
form it can be rebuilt from is about 1 MB. Real geometry images compress 50–100× (measured
across 62 images in the VCML corpus: median 73.6×, 63.5× for those over 1 MP; #2025 §3.7).
The inflated array now hangs off a
SoftReference, andgetPixels()re-inflates on demand at~700–950 MB/s.
Demonstrated under the shape of the incident
SoftPixelCacheDemo(included), 11 images all reachable at once, 256 MB heap:It's a
main, not a test — driving a JVM to the edge of its heap doesn't belong in a shared test run.Soft, not weak — the whole design
A
WeakReferenceis cleared at the next GC whatever the heap looks like, so an image in active usewould re-inflate on essentially every collection: ~90 ms of CPU, repeatedly, for nothing. Soft
references are cleared only under real pressure.
Nothing about ordinary behaviour would look wrong if someone swapped the type, so
ordinaryGcDoesNotDiscardThePixelsexists to fail if they do. Verified by temporarily switchingto
WeakReference:That control also improved the tests: with
WeakReferencein place my reflection helper threwClassCastExceptionand turned two unrelated tests into errors, burying the one failure thatactually explains the problem. The helper is now typed
Reference<?>.Two details worth review
A race in
getPixels()that had to be designed out. It takes a strong local reference once andholds it for the whole method. Reading the
SoftReferencetwice would let the collector clear itbetween the null check and the return, handing the caller a null array — rare,
non-deterministic, and extremely unpleasant to diagnose.
Two loops hoisted, two deliberately not.
ClientRequestManager:1169(reached asgetGeometrySpec().getImage()— the stored image) andImageFile:149/153callgetPixels()everyiteration and can receive a compressed image; under the very pressure that clears a soft reference
they would have re-inflated per iteration.
ROIMultiPaintManager:2036andITextWriter:544arenot touched — their receivers are locally built
VCImageUncompressedinstances, which holdpixels in a
finalfield and never inflate. (I nearly rewrote theITextWriterindex arithmeticbefore checking; it would have been risk for no benefit.)
Escape hatch
vcell.image.softPixelCache=falserestores strong caching.Verification
VCImageCompressedSoftCacheTest: 7/7 — content correctness, cache identity, soft-vs-weak,re-inflation after clearing,
nullifyUncompressedPixels, the compressed form staying held, andthe escape hatch.
MathGen_IT: 1045 tests, 0 failures, 0 errors (705 math-generation comparisons over realstored VCML).
vcell-coreFast: 587 run, 0 failures, 8 errors — theMathOverrideRoundTripTest/VCellDataTesterrorsdocs/BUILDING.mddocuments for a worktree without the Poetryenvironments.
git diff --statreads59 insertions / 11 deletions, not a whole-file rewrite).
What this does not address
The per-pixel access path.
VCImage.getPixel(x,y,z)isgetPixels()[index], andImageSubVolume.isInsidegoes throughGeometrySpec.getUncompressedPixels()one pixel at a time.Both are cold today (#2025 §3.8 traced the only live caller to a 100-point curve check), but if
either ever becomes hot they want a bulk or hoisted accessor rather than a per-call fetch.
Refs #2021, #2025, #2026
🤖 Generated with Claude Code
https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt