perf(geometry): instrument geometry-parse memory, and write down where it goes - #2023
Draft
jcschaff wants to merge 2 commits into
Draft
perf(geometry): instrument geometry-parse memory, and write down where it goes#2023jcschaff wants to merge 2 commits into
jcschaff wants to merge 2 commits into
Conversation
…e it goes
Measures rather than guesses. GeometryMemoryProfiler drives the exact path an api
request takes when it deserializes an image geometry -- VCImage, setImage,
createSampledImage, RegionImage with and without surfaces -- and reports three
different numbers per phase, because they answer three different questions:
peak high-water heap needed to finish the phase (MemoryPoolMXBean, peak reset
at phase entry). Decides whether the JVM survives.
retained live heap still held afterwards, across a settled GC with every
intermediate explicitly held. Decides how many requests fit at once.
allocated bytes the thread allocated. Decides collector pressure.
A warmup pass runs first and is discarded; without it the first measured phase absorbs
class loading and JIT and reads tens of MB high. The retention numbers hold every
intermediate on purpose -- an earlier draft let them fall out of scope and reported the
resulting collection as a memory saving.
The headline result contradicts the framing in #2021. That issue described eleven
parses of a 61,920,000 pixel geometry exhausting a 1000 MB heap. Measured at the same
pixel count:
RegionImage + surfaces peak 1,431 MB retained 450 MB allocated 1,350 MB
whole parse peak 1,431 MB retained 810 MB allocated 1,962 MB
ONE parse peaks at ~1.4 GB against a 1000 MB heap. The pods were never going to survive
that request; the repetition only decided how far it got first.
Why eleven: Xmlproducer:1416 writes the full <Geometry>, image included, inside EACH
<SimulationContext>, and XmlReader:6024 parses each independently. The prod request
(GET /api/v0/biomodel/101963252/simulation/98916046, from the PetalBot crawler) had 15
simulation contexts. So the largest available win is not in the geometry algorithms at
all -- it is parsing the geometry once per document.
A heap histogram at prod scale names the rest: 497 MB of int[] is mapImageIndexToLinkRegion
at one int per pixel (236.9 MB each), against a measured 16,706 link regions that fit in
two bytes -- and RegionImage:86-146 already carries a commented-out CompactUnsignedIntStorage
that does exactly that promotion. 480,200 quadrilaterals carry ~180 MB of Node /
Quadrilateral / MembraneEdgeNeighbor objects, ~375 bytes per quad for 16 bytes of content.
docs/geometry-memory-notes.md has the full tables, the scaling sweep from 262 K to 62 M
pixels, and six opportunities ordered by size, each with what it would cost and -- for the
per-region BitSet and the slab-wise streaming rewrite -- why it is NOT an unconditional win.
Every line-number citation was checked against this branch.
No production code changes here. This commit is the measurement and the write-up; the
optimisations are separate decisions.
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 22, 2026
…y stored Replaces the load-time veto this branch previously carried. That veto was wrong in a way only backward compatibility exposes: GeometrySpec.vetoableChange fires when DESERIALIZING a stored geometry, and the constructor turns PropertyVetoException into RuntimeException, so any enforced ceiling makes stored models above it impossible to OPEN. The model from #2021 is 61,920,000 pixels and loads today -- the earlier 50 M ceiling would have broken exactly the model it was meant to protect against. So the load path only warns, and the limits apply where a NEW image is submitted: ServerDocumentManager.saveGeometry / saveBioModel / saveMathModel, keyed on image.getKey() == null. Anything already in the database is grandfathered forever. That split is what lets the limit sit at a value the api can actually serve rather than at the largest thing anyone ever stored. Two limits, both properties: vcell.geometry.newImageSizeLimit 16,000,000 px vcell.geometry.newImageRegionLimit 2,000 regions The second one is NOT the pixel-class limit that was discussed. A class limit was implemented, measured, and abandoned, because the measurement says it is the wrong quantity. On a 256^3 volume, one subvolume per concentric shell, regions only: pixel classes 2 4 16 32 64 128 regions 2 4 16 32 64 14,050 peak 124MB 196MB 244MB 209MB 300MB 1,652MB Memory is FLAT in pixel-class count -- a 64-subvolume geometry is unremarkable at 300 MB. The jump at 128 is not the class count: at that resolution the shells fall below one voxel thick and FRAGMENT, and it is the 14,050 resulting regions that cost 1.65 GB. A 16-class limit would have rejected the cheap 64-subvolume case and still admitted a fragmented 2-class one; measured separately, a random 2-class image costs 2,111 MB at under 1 MP. Region count is also the better test for "was this image ever segmented", since a fragmented segmentation is exactly what an unsegmented image produces. It costs nothing to check: the geometry already computed a RegionImage while being parsed. RegionImage does already refuse >65535 regions, but only after doing the work, and 65535 is far above where memory turns. The size limit is a statement about today's implementation, not about the science: measured peak for one parse is 152 MB at 4.1 M px, ~450 MB at 16.8 M, 1,431 MB at 62.1 M, against a 1000 MB prod api heap that serves everything else too. Raise it once the RegionImage memory work in #2023 lands. checkNewImageAcceptable returns a reason string rather than throwing, so the server save path, the REST layer and the desktop client can all use it without agreeing on an exception type. An uncomputed region count (-1) skips the region check rather than guessing. Tests (11) pin both directions: that an image over the submission limit still LOADS and that 64 pixel classes still load -- the backward-compatibility guarantees -- as well as the refusals. manySubvolumesAreNotTreatedAsFragmentation is the one that would have failed under the abandoned class limit. vcell-core Fast: 586 run, 0 failures, 8 errors -- the MathOverrideRoundTripTest and VCellDataTest errors docs/BUILDING.md documents for a worktree without the Poetry environments. Fixes #2021 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 23, 2026
…etry Jim's objection to the first commit was correct and is the reason for this one: if five applications share one Geometry object, editing one edits all of them, which is not what anyone expects. That risk was flagged but not resolved. Resolving it turns out to cost nothing. Measured, 11 applications over one 7872x7872 image (61,968,384 px -- the pixel count from #2021): peak retained time Geometry objects VCImage objects share nothing 787.1 MB 661.4 MB 2689ms 11 11 share images only 162.1 MB 60.4 MB 287ms 11 1 share whole geometries 162.1 MB 60.3 MB 273ms 1 1 Sharing images alone captures the ENTIRE win. Sharing the Geometry on top of it buys nothing measurable and carries the aliasing risk, so it is now off by default and opt-in via vcell.xml.shareIdenticalGeometries, for read-only consumers that parse a document to serialise it or to generate math and never edit it. Sharing a VCImage is safe in a way sharing a Geometry is not: its compressed pixels are final, VCPixelClass is declared Immutable and has no setters, and everything editable -- subvolumes, their names, extent, origin, surfaces -- lives on the per-application GeometrySpec and stays private. The test demonstrates that rather than asserting it structurally: it renames a subvolume in one application and checks the other is unchanged. The measurement also corrects what the incident was. 661 MB retained over 11 parses is 11 x 62 MB of pixels, not 11 surface rebuilds -- and it matches the ~370 ms spacing of the prod warnings, which is hex-decode-plus-inflate time, not the ~1.1 s a 62 MP RegionImage takes. Reading the code confirms why: XmlReader:2086 passes the geometry into getGeometrySurfaceDescription, which MUTATES it (setGeometricRegions, setVolumeSampleSize, setFilterCutoffFrequency); only the return value is discarded, into a variable named 'dummy'. So getGeometricRegions() is non-null afterwards and precomputeAll(...,false,false) skips updateAll() entirely. A saved model reuses its stored surfaces and never rebuilds them. docs/geometry-memory-notes.md, added by #2023, said the opposite; it is corrected on that branch, not here, since the file does not exist on this one. Likely side benefit, from code reading and NOT verified against the database: saveBioModel keys memoryToDatabaseHash by the image object, and VCImage does not override equals or hashCode. Before this change, the XML round-trip inside the save produced N distinct image objects for one image, so a NEW image (key == null) fell into the insert branch N times and was written as N rows with mangled unique names. One shared object collapses that to one insert. Worth someone checking prod for duplicate image rows. Tests (6): identical images decode once; each application keeps its own Geometry, proven by renaming a subvolume; the shared image keeps its content, checked on the SECOND application because that is the one served from the cache; different images are NOT conflated; the escape hatch works; geometry sharing stays off unless asked for. MathGen_IT: 1045 tests, 0 failures, 0 errors -- 705 math-generation comparisons over real stored VCML. vcell-core Fast: 581 run, 0 failures, 8 errors -- the environmental Poetry ones. Refs #2021, #2023 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
It does not, for any saved model. XmlReader:2086 passes the geometry into getGeometrySurfaceDescription, which MUTATES it -- setVolumeSampleSize, setFilterCutoffFrequency, setGeometricRegions -- and only the RETURN value is discarded, into a variable named 'dummy'. The name is what misled me. Because getGeometricRegions() is non-null afterwards, precomputeAll(factory,false,false) skips updateAll() entirely and the stored surfaces are reused. So opportunity 2 was largely already true, and the phase-5 figure (1.4 GB peak), while a correct measurement of new RegionImage(...), is NOT a cost an XML read of a stored model pays. This also re-explains the #2021 incident: the eleven parses cost eleven image decodes and eleven retained copies of the pixels, not eleven surface rebuilds. That matches the ~370 ms spacing of the prod warnings -- hex-decode-plus-inflate time, not the ~1.1 s a 62 MP RegionImage takes -- and it is why sharing the decoded VCImage alone recovers the entire win in #2024. Refs #2021, #2024 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt
jcschaff
marked this pull request as draft
August 23, 2026 21:42
Member
Author
|
Converted to draft. Superseded for now by the planning document in #2025, which folds in this branch's measurements and corrects one claim from its notes file (XML read does not rebuild surfaces — Worth saying: the profiler in here is the piece most worth keeping regardless of which direction is chosen. Every number in #2025 came from it, and all four corrections in its section 4 were only findable because it existed. |
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.
Follow-on to #2021 / #2022. That pair asks "should an oversized image be refused"; this asks
why it costs so much in the first place, which is the question Jim raised.
No production code changes. This is the measurement plus the write-up
(
docs/geometry-memory-notes.md); the optimisations are separate decisions.The headline contradicts #2021
#2021 described eleven parses of a 61,920,000 pixel geometry exhausting a 1000 MB heap.
Measured at the same pixel count (396³ = 62,099,136):
One parse peaks at ~1.4 GB against a 1000 MB heap. Not eleven — one. The pods were never
going to survive that request; the repetition only decided how far it got first.
Why eleven, and why that is the biggest lever
Xmlproducer.java:1416writes the full<Geometry>element — image included — inside each<SimulationContext>, andXmlReader.java:6024parses each one independently. The prod request(
GET /api/v0/biomodel/101963252/simulation/98916046, from the PetalBot crawler) logged 15getting simulation context replines and 11 image warnings.So the largest available win is not in the geometry algorithms at all — it is parsing the
geometry once per document. 11× on every number in that table, plus an ~11× cut in VCML size
for multi-SimulationContext spatial models.
What is actually on the heap
jcmd GC.class_histogramat prod scale:int[]byte[]MembraneEdgeNeighborMembraneElementIdentifierNode/Node[]/ArrayList/SurfAndFace/QuadrilateralTwo things fall out:
int[]ismapImageIndexToLinkRegion— one int per pixel, 236.9 MB each —against a measured 16,706 link regions, which fit in two bytes.
RegionImage:86-146alreadycarries a commented-out
CompactUnsignedIntStoragethat does exactly this byte→short→intpromotion. Someone reached this conclusion before; it never got wired in.
geometric content is four node indices (16 bytes).
The instrument
GeometryMemoryProfilerreports three numbers per phase because they answer three differentquestions: peak (does the JVM survive), retained (how many concurrent requests fit),
allocated (collector pressure).
Two details that were needed to make the numbers trustworthy:
loading and JIT — the initial draft reported 7.9 MB retained and 41 MB allocated for
new VCImageUncompressed, which actually costs 808 bytes.and reported the resulting collection as a memory saving.
A
censusstep reads the label arrays by reflection — no accessor added to production code — toreport the actual link-region count, which is what grounds the int→short claim rather than
assuming it.
I also deleted a hardcoded "bytes per polygon" constant I had initially put in the census: it was
a guess about object layout dressed up as a measurement. Those bytes now come from the histogram.
Opportunities (full detail in the doc, ordered by size)
XmlReader:2090runsprecomputeAllon every parse;an API request that serializes a model back out never uses surfaces. Note
XmlReader:2086already parses the document's stored
<SurfaceDescription>into a variable nameddummyanddiscards it, immediately before recomputing surfaces from scratch.
written and commented out.
SurfaceCollection,TaubinSmoothing, the STL exporter and the VTK path. Worth costing first.and the surface pass already streams two z-planes of nodes, so the structure is largely there.
createSampledImagecopying a fullbyte[]when it could alias the original;getShortEncodedRegionIndexImagebuilding abyte[2N](124 MB here) that callers could stream.Where an idea is not an unconditional win, the doc says so — the per-region
BitSetalternative (also commented out, at
RegionImage:880-885) isnumDistinctRegions × Nbits, whichis 23 MB for a clean 3-region segmentation and catastrophic for a noisy one. That is very likely
why it was commented out, and it should be chosen at runtime from the measured region count if
it is used at all.
One piece of dead weight found, deliberately not fixed here
FloodFill2DLine(RegionImage:213) has an inverted guard —if (sp + 4 < MAXDEPTH_TIMES_4) throw new RuntimeException("stack overflow")— which would throwwhenever there is room. It never fires because the code is unreachable: its only caller
calculateRegions3Dfasteris invoked solely from a commented-out line (RegionImage:554), insidecalculateRegions, itself commented out atRegionImage:407. Worth deleting so nobody loses timeon it, but it is not a live defect and does not belong in this PR.
Verification
Every line-number citation was checked against this branch (they are
master+#1997; noteGeometrySpecshifts ~+26 lines once #2022 lands, which the doc records).mvn test-compile -pl vcell-coreis clean. The profiler is amainin test sources with no@Test, so CI does not runit.
Reproducing the prod-scale figure needs
-Xmx6gand, once #2022 lands,-Dvcell.geometry.imageSizeLimitraised — the ceiling refuses the image, which is exactly what it is for. That refusal firing during
this work was the first live confirmation that #2022 behaves as designed.
Refs #2021, #2022
🤖 Generated with Claude Code
https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt