perf(xml): share the decoded image between applications, not the Geometry

[jcschaff]

Implements the largest opportunity identified in #2023: parse each distinct geometry once per
document instead of once per application.

The problem

A BioModel stores a full copy of its geometry — image included — inside every
<SimulationContext>. Xmlproducer.java:1416 writes it there; XmlReader.java:6024 parses each
copy independently. A model with 11 spatial applications on one geometry paid for that geometry 11
times on every read.

That is what killed two prod api pods (#2021): GET /api/v0/biomodel/101963252/simulation/98916046
from the PetalBot crawler. The model has 15 simulation contexts, and the log shows 11 image-size
warnings — one per geometry parse — before the heap ran out.

What changed

XmlReader digests each <Geometry> element and reuses the Geometry it already parsed for an
identical one. The cache is a field on XmlReader, and XmlHelper constructs one XmlReader per
document, so it lives exactly as long as one parse and is never shared across documents or threads.

Measured

XmlGeometrySharingBenchmark (included), 11 applications over one 7872×7872 image —
61,968,384 pixels, the pixel count from the incident:

	peak	retained	time
sharing OFF (before)	790.9 MB	661.4 MB	2626 ms
sharing ON (after)	162.0 MB	60.3 MB	303 ms
	4.9×	11.0×	8.7×

Retained falls by exactly the application count, which is the shape you'd expect if the fix does
what it claims.

790 MB is a lower bound on what prod was doing. The benchmark's 2D two-subvolume image is far
cheaper than the real 3D model — the profiler in #2023 measured ~1.4 GB peak for ONE parse of a
62 MP 3D geometry. Against a 1000 MB heap.

Scope of the sharing

Only byte-identical elements share. Nothing here tries to decide that two different
geometries are equivalent — that is a much harder question and not one a parser should be
answering.

The digest streams the element through SHA-256 rather than building a String first: the element
carries the image as hex text, so for this model that String would be tens of MB, and allocating
it to avoid allocations would be self-defeating. If the digest can't be computed the element is
simply parsed unshared — a failure there costs performance, never correctness.

On sharing a mutable object — the part worth your attention

This changes object identity, so it deserves a straight answer rather than a reassurance. What I
checked:

• A SimulationContext and its MathDescription already share one Geometry instance today
  (ParticleMathMapping:456 among others), so intra-document aliasing is established practice, not
  a new idea.
• Geometry holds no back-reference to an owner — its fields are version, name, description,
  GeometrySpec, unit system, GeometrySurfaceDescription.
• The client's editing paths replace a geometry (SimulationContext.setGeometry, e.g.
  GeometryViewer:197, ClientRequestManager:259) rather than mutating one in place.

The residual risk is a client path that mutates a geometry in place — renaming a subvolume, say —
where the change would now be visible to sibling applications. I did not find one that applies to a
simulation context's geometry, but I can't prove absence across the whole client. Hence
vcell.xml.shareIdenticalGeometries=false restores the old behaviour: an escape hatch, not a
tuning knob.

If you'd rather this were narrower, the conservative variant is to share only the immutable heavy
payload (VCImage, RegionImage, SurfaceCollection) and still build a GeometrySpec per
application. It's more code and gives up part of the win. Your call — this is your architecture.

Tests

XmlReaderGeometrySharingTest, 4 cases:

• identical elements share
• different geometries are NOT conflated — the important one. A key that stopped discriminating
  would be far worse than the memory problem being solved.
• the shared geometry keeps its image, subvolumes and surfaces — checked on the second
  application, because that is the one served from the cache
• the property restores the old behaviour

That last test doubles as the automated negative control: it asserts distinct instances with
sharing off, which is exactly the pre-change behaviour. The suite therefore proves both directions
without needing a manual revert.

Regression

• MathGen_IT: 1045 tests, 0 failures, 0 errors (705 MathGenCompareTest + 340
  MathOverrideApplyTest, 13 min). These parse real stored VCML biomodels and compare generated
  math — the strongest available signal that sharing a Geometry changes nothing semantically.
• vcell-core Fast: 579 run, 0 failures, 8 errors — the MathOverrideRoundTripTest /
  VCellDataTest errors docs/BUILDING.md documents for a worktree without the Poetry
  environments.

Not included

The write side. Emitting geometries once at BioModel level and referencing them by key would cut
VCML size ~11× for these models too, but it is a format change with old-client implications and
belongs in its own discussion.

Refs #2021, #2023

🤖 Generated with Claude Code

https://claude.ai/code/session_018kr8SbzXtwW3gMVUgMfDDt

[jcschaff]

You're right, and I'd flagged that risk without resolving it. Resolving it turns out to cost nothing — pushed 0895b6ec1a.

Measured all three, 11 applications over one 7872×7872 image (61,968,384 px — the incident's pixel count):

	peak	retained	time	Geometry objects	VCImage objects
share nothing	787.1 MB	661.4 MB	2689 ms	11	11
share images only	162.1 MB	60.4 MB	287 ms	11	1
share whole geometries	162.1 MB	60.3 MB	273 ms	1	1

Sharing images alone captures the entire win. Sharing the Geometry on top buys nothing measurable and carries exactly the aliasing you described, so it's now off by default and opt-in via vcell.xml.shareIdenticalGeometries for read-only consumers (a server parsing to serialise or generate math).

Sharing a VCImage is safe in a way sharing a Geometry is not: compressed pixels are final, VCPixelClass implements Immutable with no setters, and everything editable — subvolumes, their names, extent, origin, surfaces — lives on the per-application GeometrySpec. The test demonstrates it rather than asserting it structurally: renames a subvolume in one application, checks the other is unchanged.

The measurement also corrected what the incident was. 661 MB over 11 parses is 11 × 62 MB of pixels, not 11 surface rebuilds — matching the ~370 ms warning spacing (hex-decode + inflate), 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 goes into the variable named dummy. So getGeometricRegions() is non-null and precomputeAll(...,false,false) skips updateAll(). A saved model reuses its stored surfaces. I'd claimed the opposite in #2023 — corrected there in 8572000ccf.

Possible side benefit, code-reading only, not verified against the database: saveBioModel keys memoryToDatabaseHash by the image object, and VCImage doesn't override equals/hashCode. The XML round-trip inside the save produced N distinct objects for one image, so a new image (key == null) hit the insert branch N times and was written as N rows with mangled unique names. One shared object collapses that to one insert. Might be worth checking prod for duplicate image rows.

MathGen_IT 1045 tests / 0 failures; vcell-core Fast 581 / 0 failures + the 8 environmental.

[jcschaff]

Converted to draft. Superseded for now by the planning document in #2025, where this appears as option §5.1 — the recommended first step, and the only one with the full measured win already demonstrated (peak 787→162 MB, retained 661→60 MB, 8.7× faster, MathGen_IT green). Not merging yet; sequencing and the format question in §5.2 should be settled first.

[jcschaff]

Closing. The premise this was built on no longer holds, and two of the numbers in the description
above are wrong.

It is not needed for the memory problem

This was designed when the incident looked like 11 × 62 MB of unavoidable retention. #2026 and
#2027 removed that premise. SoftPixelCacheDemo reproduces the incident's shape — 11 images all
reachable at once, 256 MB heap:

soft cache OFF   OUT OF MEMORY after 9 of 11
soft cache ON    completed all 11

Each parsed geometry still holds its VCImage strongly, but the image now holds ~1 MB of
compressed bytes strongly (median 73.6× compression across the corpus) with the inflated array
softly reachable. The retained floor for the incident drops from ~680 MB to ~11 MB without
deduplicating anything.

The measured win was overstated

The benchmark above used 11 applications over one identical image — synthetic, identical by
construction. Real stored models are not. Across 23 corpus models carrying 103 image elements:

identity notion	decodes	saved
today	103	—
this PR (digest of the whole <Image>)	66	36%
by database key	66	36%
by pixel payload	52	50%

biomodel_27192717 is the clearest case: 9 image elements, 8 distinct names, 8 distinct keys, but
only 2 distinct pixel payloads. This PR shares almost none of it — because the duplicate-insert
bug it was partly meant to help had already renamed every copy
(purkinje9_3D_crop1017221890, …1846473978) and given each its own row. The name and version sit
inside the element being digested, so the bug defeats its own detection.

The design objection stands

The digest matches neither identity that means anything:

• not the declared identity — VCML does carry KeyValue, and that is what
  ServerDocumentManager effectively uses (memoryToDatabaseHash, keyed by object reference);
• not content identity — payload alone would share 9→2, but that merges rows the database
  considers distinct. A data-model decision, not a parser optimisation.

And sharing instances needs a client-side split on write, which this has no mechanism for. Even
confined to VCImage, setName / setPixelClasses / setDescription mutate the shared instance,
and ServerDocumentManager calls setName on images in four places during save
(saveBioModel:840, saveGeometry:1577, saveMathModel:1717, saveSimulation:2128).

What replaces it

An explicit list of geometries on the BioModel, edited directly — sharing as a modelled,
user-visible concept rather than something a parser infers. That gives declared identity for the
save controller to key on, a natural home for copy-on-write (the user copies a geometry, visibly),
and it matches what the database already does; VCML is the layer that discards the relationship by
inlining. Recorded as §5.2 in #2025.

The one genuinely useful thing here — the duplicate-insert fix — deserves its own small PR rather
than arriving as a side effect. VCImage overrides neither equals nor hashCode while
saveBioModel keys a hashtable by it; that is a targeted defect with a small blast radius.

Superseded by #2025 §5.2. Not deferred — the reasoning is recorded so nobody re-derives it.

Refs #2021, #2025, #2027