Skip to content

docs(geometry): consolidate the geometry-memory incident, measurements and options - #2025

Open
jcschaff wants to merge 6 commits into
masterfrom
docs/geometry-memory-plan
Open

docs(geometry): consolidate the geometry-memory incident, measurements and options#2025
jcschaff wants to merge 6 commits into
masterfrom
docs/geometry-memory-plan

Conversation

@jcschaff

Copy link
Copy Markdown
Member

Planning document, deliberately not a fix.

The 2026-08-22 prod OOM produced three prototype branches in quick succession, and the understanding
changed three times while they were being written. That is the signal that this needs planning
rather than merging, so #2022, #2023 and #2024 are now drafts and this document is where
decisions should happen.

What it contains

  • §1–2 what happened, and the root cause: Xmlproducer:1416 writes the complete <Geometry>
    image included — inside every <SimulationContext>, so 11 applications means 11 decodes and
    ~680 MB of retained pixels against a 1000 MB heap.
  • §3 every measurement, with the instrumentation that produced it — parse cost by sharing mode,
    RegionImage scaling, a heap class histogram, a comparison against scipy.ndimage.label /
    cv2.connectedComponents, the pixel-class experiment, and the prod duplicate-row queries.
  • §4 what we got wrong. Four beliefs that were acted on and then disproved, two of which
    reached open PRs.
  • §5 nine options, each with what it buys, what it costs, and what is unresolved.
  • §6 the open questions that block implementation.

The correction that matters most

XML read does not rebuild surfaces. XmlReader:2086 assigns the parsed surface description to a
variable named dummy, which reads like the stored surfaces are thrown away — but the method
mutates the geometry it is passed and only the return value is discarded. So
getGeometricRegions() is non-null and precomputeAll skips updateAll() entirely.

The incident was eleven image decodes and eleven retained pixel arrays, not eleven surface
rebuilds. That matches both the measured 661 MB and the ~370 ms spacing of the prod warnings —
hex-decode-plus-inflate time, not the ~1.1 s a 62 MP RegionImage takes.

Also recorded: a pixel-class limit was implemented, measured and abandoned (memory is flat in
class count; region count is the predictor); enforcing the image-size veto on the load path would
have made BioModel 101963252 — the very model from the incident — impossible to open; and the first
duplicate-row query over-matched by 13×.

Two things that are ready to act on independently

  • Prod has ~944 surplus image rows from same-save duplication, confirmed with a 1:1
    image-row-per-referencing-geometry signature, and still accruing (October 2025).
  • The trigger is crawler traffic. A robots.txt or rate limit on the legacy api removes it
    without touching geometry code.

Not decided here

Both limits in the #2022 prototype (16 M px, 2,000 regions) are calibrated against today's
implementation, not against what the science needs. §6 lists the read-only queries that would
replace those guesses with data.

Supersedes docs/geometry-memory-notes.md from #2023.

Refs #2021, #2022, #2023, #2024

🤖 Generated with Claude Code

https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt

…s and options

Planning document, deliberately not a fix. The 2026-08-22 prod OOM produced three
prototype branches in quick succession (#2022, #2023, #2024) and the understanding
changed three times while they were being written -- which is the signal that this
needs planning rather than merging.

Records what happened, what was measured, what turned out to be WRONG, and the
options with their costs. Section 4 is the part worth reading: four beliefs that
were acted on and then disproved, including two of my own that reached open PRs.

The most consequential correction: XML read does NOT rebuild surfaces. XmlReader:2086
assigns the parsed surface description to a variable named 'dummy', but the method
MUTATES the geometry it is passed and only the return value is discarded, so
getGeometricRegions() is non-null and precomputeAll skips updateAll entirely. The
incident was eleven image decodes and eleven retained pixel arrays (~680 MB against
a 1000 MB heap), not eleven surface rebuilds -- which matches both the measured
661 MB and the ~370 ms spacing of the prod warnings.

Also records that a pixel-class limit was implemented, measured and abandoned
(memory is flat in class count; region count is the predictor), that enforcing the
image-size veto on the load path would have broken the very model it protected, and
that the first duplicate-row query over-matched by 13x.

Supersedes docs/geometry-memory-notes.md from #2023.

Refs #2021, #2022, #2023, #2024

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
jcschaff and others added 2 commits August 23, 2026 18:27
Jim's suggestion: VCImageCompressed already keeps compressed pixels strongly and
the uncompressed copy as a transient cache, so let the uncompressed copy become
reclaimable, and consider holding other large objects compressed too. Measured
before writing it up, and the numbers are better than expected.

To the direct question first: RegionImage is NOT a VCImage. It implements
Serializable, takes a VCImage as a constructor argument and keeps only the
dimensions. Its heavy state is its own int[] label map plus the SurfaceCollection.
So the idea applies in two separate places, not one.

Real geometry images out of the VCML test corpus -- 62 of them, using the
CompressedSize already recorded in <ImageData>, so no synthetic stand-in whose
compressibility would be wrong by construction:

  all 62 images       23.3x .. 73.6x median .. 287.7x
  the 32 >= 1 MP      53.6x .. 63.5x median .. 287.7x
  aggregate           83.8 MB raw -> 1.1 MB compressed (78.8x)

Rehydration is cheap: 676-951 MB/s across four real images, so a 62 MP image
re-inflates in roughly 65-90 ms against the ~2.7 s the eleven-application parse
takes today.

The result that was not obvious: the DERIVED label array compresses too, 53-93x
as int[]. Scaled to 62 MP, mapImageIndexToLinkRegion -- the single largest
retained object at 236.9 MB -- becomes 2.5-4.5 MB.

One correction that decides whether the idea works: use SoftReference, not
WeakReference. A weak reference is cleared at the next GC regardless of memory
pressure, so a hot geometry would re-inflate on essentially every collection --
90 ms of CPU repeatedly for no benefit. Soft references are cleared only under
actual pressure.

Three verified blockers for a naive version: GeometrySpec.uncompressedPixels
holds a SECOND strong reference to the same array and would pin it regardless;
VCImageUncompressed has no compressed form at all (final byte[]), which matters
because the sampled image is one; and a compressed label map is not randomly
accessible while isIndexInRegion needs random access, so it wants block-wise
compression or RLE.

Feasibility rests on an audit of getPixels(), which returns the internal array
and has 178 call sites. A narrow check of cbit.image and cbit.vcell.geometry
found no mutation, but that is a small fraction of the surface.

Also concedes a point: the earlier objection that tiling bounds the working set
while the finished artefact still has to fit in memory is weakened by this. If
the result is held compressed, size stops being the binding constraint. The
remaining obstacles to stitching are about correctness, not size.

Refs #2021, #2025

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
…than claimed

Resolves the open item that decided whether the compressed-and-rehydrate idea in
5.3b is feasible.

Method matters here, because grep cannot answer this question: getPixels() also
exists on ByteImage, UShortImage, FloatImage, ShortImage and ImageDataset, and a
text search cannot tell which receiver a call is on. The compiler already resolved
every receiver, so the call sites were read out of the BYTECODE with javap across
all 13 built modules and then located in source. (I also tried mapping bytecode
offsets to lines via LineNumberTable; it put several call sites on lines with no
call at all, so that step was dropped rather than trusted.)

Corrects my own figure. This document said 178 call sites. That was a raw grep
count dominated by the unrelated image classes. Real surface: 39 call instructions
across 22 files, about 31 distinct source expressions.

Classification: ~27 read-only and transient, 2 already defensively .clone(),
3 that retain the array, and exactly ONE that mutates it.

The single mutation is DatabaseWindowManager:1004, and it is on a
VCImageUncompressed built locally by createSampledImage for display scaling --
never a stored, compressed image. So it does not block softening
VCImageCompressed.uncompressed. It does prove that returning the LIVE array is
load-bearing: any variant returning a defensive copy silently loses those writes.

Of the three retention sites only one matters -- GeometrySpec:962, which stores the
stored image's array in a field. Now confirmed to be the only one of its kind. The
other two (SourceDataInfo, MemoryImageSource) hold the SAMPLED image's array, which
has no compressed form to soften against.

The finding I did not expect is a hazard that is neither mutation nor retention:
four loops call getPixels() in the loop header, and deeper than that,
ImageSubVolume.isInside calls getUncompressedPixels()[index] for ONE PIXEL PER CALL
from sampling loops. Under the memory pressure that clears a soft reference --
exactly the condition this is for -- that turns a pointer dereference into an
inflate. That, not correctness, is the real design problem.

Refs #2021, #2025

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
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

Copy link
Copy Markdown
Member Author

Updated for §5.3b being built (b87d1ce026).

New §3.9 — the end-to-end check that §3.7's compression arithmetic actually changes the outcome.
11 images, 300³ each, 256 MB heap:

result
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. The incident and its fix in one run.

§5.3b rewritten as built#2026 (removes the second strong reference) and #2027 (the
SoftReference itself, stacked on it). Both open, neither merged.

I was deliberate about what the section does not claim, 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;
    getPixel(x,y,z) and 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 corrected two places in §3.8 that still read as open, added a state column to the branch table
now that drafts and open PRs are mixed, and expanded the repro commands to include
SoftPixelCacheDemo (it needs its own -Xmx, so it does not go through exec:java).

The sequencing question in §6 is still open — §5.3b and §5.1 are independent of each other and of
the format question in §5.2.

#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
jcschaff added a commit that referenced this pull request Aug 24, 2026
Documentation only -- no fixture or golden is touched, so this branch stays pinned at
the pre-change implementation (f35bead) and remains usable for generating goldens
against it.

Records the workflow Jim asked for: branch from HERE to add fixtures, generate goldens
with the old code, then CHERRY-PICK the result onto master rather than merging master
into this branch. Merging would drag the new implementation in and destroy the one
property that makes this branch worth keeping.

Refs #2021, #2025

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
…tion

Records why #2024 was closed rather than deferred, so nobody re-derives it.

It is not needed. It was designed when the incident looked like 11 x 62 MB of
unavoidable retention; 5.3b removed that premise. With the pixels held softly the
retained floor for the same document drops from ~680 MB to ~11 MB, without
deduplicating anything.

The win was also overstated, and section 4.6 now records that as one of my own errors.
The 11x figure came from a synthetic document built with eleven IDENTICAL copies -- the
benchmark assumed the thing it was meant to test. Measured across 23 corpus models
carrying 103 image elements (new section 3.10):

  today                          103 decodes
  digest of the <Image> element   66   (36%)
  by database key                 66   (36%)
  by pixel payload                52   (50%)

biomodel_27192717 shows why: 9 image elements, 8 distinct names, 8 distinct keys, and
only 2 distinct pixel payloads. The duplicate-insert bug had already renamed every copy
and given each its own row, and the name and version sit INSIDE the digested element --
so the bug defeats its own detection.

And the design was wrong regardless: the digest matched neither declared identity
(KeyValue, which the save controller effectively uses) nor content identity, and sharing
instances needs a client-side split on write that it had no mechanism for.

5.2 is rewritten from a VCML format change to giving a BioModel an explicit list of
geometries, edited directly -- Jim's framing, and a better one. Modelling the sharing
answers every objection that sank 5.1 at the source: declared identity for the recursive,
non-atomic incremental save controller to key on, a visible home for copy-on-write, and
it matches what the database already stores. VCML is the layer that discards the
relationship by inlining.

New 5.10 salvages the one useful part of #2024 as its own item: VCImage overrides neither
equals nor hashCode while saveBioModel keys a hashtable by it, so a new image is inserted
once per application. Includes the finding that
SQLCreateAllTables.writeScript(POSTGRES, ...) already emits the whole schema, so a
testcontainer can build the real thing rather than a stub -- there is currently no test of
the document save path at all.

Sequencing reordered accordingly: 5.9 (free) -> 5.10 (stop creating duplicates) -> 5.8
(clean up the existing ones) -> 5.4 -> 5.2.

Refs #2021, #2024, #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
GeometrySpec.java is CRLF in master. An early text-mode edit on this branch rewrote
every line ending to LF, so the diff read 2988 changed lines for a ~90 line change --
unreviewable, and pure whitespace churn in a file with no styling standard.

Converted back in binary mode. The file was uniformly LF so the conversion is exact:
no ending was ambiguous, and the assertion that stripping CR reproduces the previous
bytes is in the script, so no content moved.

This is the same trap that later bit the pixel-retention work, where it was caught on
git diff --stat before pushing. Worth checking --stat after any scripted edit to this
tree; ServerDocumentManager.java is LF in master and was fine.

Refs #2021, #2025

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