Skip to content

fix(geometry): stop GeometrySpec pinning the image's inflated pixel array - #2026

Merged
jcschaff merged 1 commit into
masterfrom
fix/geometryspec-pixel-retention
Aug 24, 2026
Merged

fix(geometry): stop GeometrySpec pinning the image's inflated pixel array#2026
jcschaff merged 1 commit into
masterfrom
fix/geometryspec-pixel-retention

Conversation

@jcschaff

Copy link
Copy Markdown
Member

Removes the one blocker identified by the getPixels() audit (#2025 §3.8).

GeometrySpec cached getImage().getPixels() in a transient uncompressedPixels field, making it
a second strong reference to the very same array VCImageCompressed already caches. The
inflated pixels then stayed reachable for as long as the geometry did, whatever the image did with
its own copy — 62 MB pinned per geometry at the size that killed two prod api pods (#2021).

Why removing it is safe rather than a trade

The cache was redundant. VCImageCompressed.getPixels() already caches the inflated array, and
VCImageUncompressed.getPixels() returns its field directly. Delegating costs a virtual call and a
null check — not a re-inflate. The field only ever added a second reference.

And it fed a cold path. The audit established that the only caller reaching an ImageSubVolume
is getSubVolume(x,y,z), whose sole caller is curveSatisfyGeometryConstraints100 sampled
points per curve
. The other call site, GeometrySpec:898, is a diagnostic that prints
"there was no subVolume defined". Nothing here runs per-pixel over a volume.

(That mattered because the audit's other finding was a thrash hazard on per-pixel access. It turns
out not to apply to this particular field.)

Tests assert reachability, not structure

So they still catch the problem if the reference reappears somewhere other than that field:

WeakReference<byte[]> watch = new WeakReference<>(pixels);
pixels = null;
image.nullifyUncompressedPixels();   // the image releases its own cache
collect();
assertTrue(watch.get() == null, "GeometrySpec must not hold a second reference ...");

theImagesOwnCacheStillPinsThePixels is the negative control for the control — it asserts the
array is not collectable while the image still caches it. Without that, a test which always passed
because nothing ever retained the array would look like a working fix.

Verified the suite detects the defect. With the old caching field restored:

geometrySpecDoesNotPinTheInflatedPixels
  GeometrySpec must not hold a second reference to the image's inflated pixels
  ==> expected: <null> but was: <[0, 1, 2, 0, 1, 2, ...]>
Tests run: 5, Failures: 1

The other four passed. That failure also exposed a usability problem worth fixing: assertNull
rendered the whole 64,000-byte array into the message — 375 KB of output burying the actual
assertion — so the two reachability assertions now use assertTrue on an explicit comparison.

A note on the diff

GeometrySpec.java is CRLF (1445 CRLF lines, no bare LF). My first edit went through Python's
text mode and silently rewrote every line ending, turning a 10-line change into a 2900-line diff.
Redone in binary mode; git diff --stat now reads 18 insertions / 7 deletions, which is what it
should have been.

Verification

  • GeometrySpecPixelRetentionTest: 5/5.
  • MathGen_IT: 1045 tests, 0 failures, 0 errors — 705 math-generation comparisons over real
    stored VCML. Re-run clean after the binary-mode patch; an earlier run was recompiled mid-flight
    and discarded rather than reported.
  • vcell-core Fast: 580 run, 0 failures, 8 errors — the MathOverrideRoundTripTest /
    VCellDataTest errors docs/BUILDING.md documents for a worktree without the Poetry
    environments.

Relationship to the other branches

This stands on its own — it removes a redundant reference to a 62 MB array. It is also a
prerequisite for #2025 §5.3b: making VCImageCompressed.uncompressed a SoftReference would
do nothing while a second strong reference pinned the array regardless.

Unlike #2022 / #2023 / #2024, this one is small, self-contained, and does not depend on any of the
decisions still open in #2025.

Refs #2021, #2025

🤖 Generated with Claude Code

https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt

…rray

GeometrySpec cached getImage().getPixels() in a transient uncompressedPixels field,
which made it a SECOND strong reference to the very same array VCImageCompressed
already caches. The inflated pixels then stayed reachable for as long as the geometry
did, whatever the image did with its own copy -- 62 MB pinned per geometry at the size
that killed two prod api pods (#2021).

The cache was redundant. VCImageCompressed.getPixels() already caches the inflated
array and VCImageUncompressed.getPixels() returns its field directly, so delegating
costs a virtual call and a null check, not a re-inflate. The field only ever added a
second reference.

It also fed a cold path. The getPixels() audit (see #2025 section 3.8) established that
the only caller reaching an ImageSubVolume is getSubVolume(x,y,z), whose sole caller is
curveSatisfyGeometryConstraints -- 100 sampled points per curve. The other call site,
GeometrySpec:898, is a diagnostic that prints "there was no subVolume defined". Nothing
here runs per-pixel over a volume.

Behaviour is otherwise unchanged: a geometry with no image still returns null rather
than throwing, and setImage no longer has a cache to reset.

This is a prerequisite for the compressed-and-rehydrate work in #2025 section 5.3b --
making VCImageCompressed.uncompressed a SoftReference does nothing while a second
strong reference pins the array regardless. It stands on its own either way.

Tests assert REACHABILITY, not structure, so they still catch the problem if the
reference reappears somewhere other than that field: hold the array through a
WeakReference, let the image drop its own cache with nullifyUncompressedPixels(), and
require the array to become collectable. theImagesOwnCacheStillPinsThePixels is the
negative control for that -- without it, a test that always passed because nothing ever
retained the array would look like a working fix.

Verified the test detects the defect: with the old caching field restored,
geometrySpecDoesNotPinTheInflatedPixels fails with "expected: <null> but was: <[0, 1,
2, ...]>" while the other four pass. That failure also revealed a usability problem --
assertNull rendered the whole 64,000-byte array into the message, 375 KB of output
burying the actual assertion -- so the two reachability assertions use assertTrue on an
explicit comparison instead.

Note on the diff: GeometrySpec.java is CRLF (1445 CRLF lines, no bare LF). An initial
text-mode edit silently rewrote every line ending and produced a 2900-line diff for a
10-line change; this was redone in binary mode, and `git diff --stat` now reads 18
insertions / 7 deletions.

vcell-core Fast: 580 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
jcschaff merged commit 8a6e9f7 into master Aug 24, 2026
9 checks passed
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
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