#4659 Phase 1 — eliminate ConcurrencyMode runtime branches via type splits in closed-shape#4660
Merged
jeremydmiller merged 1 commit intoJun 5, 2026
Conversation
…ions / storages / selectors
Eliminates every per-operation and per-row read of
_descriptor.ConcurrencyMode in src/Marten/Internal/ClosedShape/ by splitting
each writeable closed-shape class into three sealed leaves keyed on the
mode:
Operations (each kind → 1 abstract base + 3 sealed leaves):
ClosedShape{Insert,Update,Upsert,Overwrite}Operation
→ Unversioned/Optimistic/Numeric ClosedShape{X}Operation
12 sealed leaves; bind exactly the slots their mode needs, no runtime
switch on ConcurrencyMode. Numeric variants own the UseVersionFromMatchingStream
subquery layout. Optimistic leaves carry typed Dictionary<TId, Guid>
trackers; Numeric leaves carry typed Dictionary<TId, long> trackers;
Unversioned leaves carry no tracker at all. Optimistic/Numeric Overwrite
keep the tracker nullable to preserve the #4658 OverwriteProjected
contract (projection path passes null).
Storages (each writeable kind → 1 abstract base + 3 sealed leaves):
{Lightweight,IdentityMap,DirtyChecked}ClosedShapeStorage
→ Unversioned/Optimistic/Numeric {X}ClosedShapeStorage
9 sealed leaves. Each leaf's Insert/Update/Upsert/Overwrite/
OverwriteProjected/BuildSelector factories construct the matching
concurrency-mode operation + selector directly. The shared
VersionsFor/RevisionsFor helpers are gone — each leaf knows its mode.
QueryOnlyClosedShapeStorage is unchanged (no write path).
Selectors (each tracking kind → 1 abstract base + 3 sealed leaves):
ClosedShape{Lightweight,IdentityMap,DirtyTracking}Selector
→ Unversioned/Optimistic/Numeric ClosedShape{X}Selector
9 sealed leaves. Each leaf overrides CaptureVersion with the
monomorphic per-row implementation (no-op / Guid capture / long
capture). The per-row hot path now contains zero ConcurrencyMode reads.
ClosedShapeQueryOnlySelector unchanged (no concurrency interaction).
Registration:
ClosedShapeRegistration.BuildProvider gains three small dispatch
helpers (BuildLightweightStorage / BuildIdentityMapStorage /
BuildDirtyCheckedStorage) that switch on descriptor.ConcurrencyMode
ONCE at registration time and construct the right concurrency-specific
storage leaf. The closed-shape DocumentProvider keeps its 4-tuple
shape; only the writeable members change leaf identity.
Public API impact:
IDocumentStorage<T, TId> and DocumentStorage<T, TId> are unchanged.
The three writeable storage classes (LightweightClosedShapeStorage,
IdentityMapClosedShapeStorage, DirtyCheckedClosedShapeStorage) flip
from `public sealed` → `public abstract` — they still exist as public
types but are no longer directly instantiable. Existing
ClosedShapeRegistration callers + the W3 spike's UseLightweightSequentialGuidClosedShape /
UseExternallyAssignedStringClosedShape extensions continue to work
because the registration internals build the leaves. All new leaves
are `internal sealed`.
Numeric leaves use `await reader.GetFieldValueAsync<long>(0, token)`
instead of the synchronous read the original used inside the private
ApplyConcurrencyResult helper — the VSTHRD103 analyzer flags it once
the read is hoisted into PostprocessAsync directly. Behaviorally equivalent.
DescriptorBuilder and DescriptorTorageDescriptor itself are unchanged —
ConcurrencyMode is now factory input only, not a runtime read.
Acceptance criteria (issue #4659 Phase 1):
- `grep -rn "_descriptor\\.ConcurrencyMode" src/Marten/Internal/ClosedShape/`
returns 0 hits (modulo ClosedShapeRegistration's 3 dispatch sites + 1
comment).
- Every new variant is `internal sealed`.
- No new public API surface; the three writeable storage class names
are preserved (now abstract bases).
- DocumentDbTests: 999/1000 passed locally on net9 (one unrelated
pre-existing skip).
- PatchingTests: 122/123 passed locally on net9 (one unrelated
pre-existing skip).
Phase 2 (hierarchy selector split — `HierarchyMapping is { }` per-row
branch in the four read selectors) is the planned follow-up PR per the
issue's phased plan.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jeremydmiller
added a commit
that referenced
this pull request
Jun 5, 2026
Builds on Phase 1 (#4660). Each closed-shape selector now further splits on DocumentStorageDescriptor.HierarchyMapping: each Phase-1 concurrency- mode leaf gains 2 sealed Flat/Hierarchical sub-leaves that provide a monomorphic ReadDocument / ReadDocumentAsync. The per-row hot path no longer branches on HierarchyMapping. Shape: QueryOnly (no concurrency split): ClosedShapeQueryOnlySelector (abstract) + Flat / Hierarchical sealed leaves (2) Each tracking style (Lightweight, IdentityMap, DirtyTracking): ClosedShapeXSelector (abstract — Resolve/Async + ApplyMetadata) + 3 concurrency-mode intermediates (abstract) — provide CaptureVersion - Unversioned (no-op), Optimistic (Guid capture), Numeric (long capture) + 6 sealed Flat/Hierarchical leaves per intermediate → 18 sealed tracking leaves per Phase 2 Total new sealed selectors: 18 (tracking) + 2 (QueryOnly) = 20. Total intermediate abstracts: 3 per tracking style × 3 styles = 9 (these REPLACE the Phase-1 concrete sealed leaves — same names, just promoted to abstract intermediates that hold the concurrency-specific CaptureVersion impl). Per-storage-leaf BuildSelector now dispatches on _descriptor.HierarchyMapping once per query (10 storage leaves modified — 9 tracking + 1 QueryOnly). Acceptance criteria: $ grep -rn "HierarchyMapping is " src/Marten/Internal/ClosedShape/ → 10 hits, all in storage BuildSelector factory dispatches. Per-row ReadDocument / ReadDocumentAsync no longer reads HierarchyMapping at all — each leaf is one strategy. Local tests: DocumentDbTests.Hierarchical.*: 31/31 pass. DocumentDbTests: 999/1000 pass (1 unrelated pre-existing skip). The Hierarchical leaves capture the HierarchyMapping + DocTypeReadIndex in their constructors with a null-guard ArgumentException so a future misrouted factory call would fail fast rather than NRE on first row. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
First of the two PRs the #4659 phased plan calls for.
What this changes
Each writeable closed-shape class in
src/Marten/Internal/ClosedShape/is split into a small abstract base + threeinternal sealedleaves keyed on the document'sConcurrencyMode(Off→Unversioned…/Optimistic/Numeric). After the split, zero per-operation or per-row reads of_descriptor.ConcurrencyModeremain — the storage class is monomorphic by construction.Operations (12 sealed leaves + 4 abstract bases)
UnversionedClosedShapeInsertOperationOptimistic…Numeric…UnversionedClosedShapeUpdateOperationOptimistic…Numeric…UnversionedClosedShapeUpsertOperationOptimistic…Numeric…UnversionedClosedShapeOverwriteOperationOptimistic…Numeric…Each leaf binds exactly the slots its mode needs. Optimistic leaves carry
Dictionary<TId, Guid>trackers; Numeric leaves carryDictionary<TId, long>trackers; Unversioned leaves carry none. The Numeric leaves own theUseVersionFromMatchingStreamsubquery shape. The Optimistic / Numeric Overwrite leaves keep the tracker nullable to preserve the #4658OverwriteProjectedcontract.Storages (9 sealed leaves + 3 abstract bases)
UnversionedLightweightClosedShapeStorageOptimistic…Numeric…UnversionedIdentityMapClosedShapeStorageOptimistic…Numeric…UnversionedDirtyCheckedClosedShapeStorageOptimistic…Numeric…QueryOnlyClosedShapeStorageis unchanged — no write path, no concurrency interaction.The shared
VersionsFor/RevisionsForhelpers are gone; each leaf knows its mode.Selectors (9 sealed leaves + 3 abstract bases)
ClosedShape{Lightweight, IdentityMap, DirtyTracking}Selectoreach becomes an abstract base + 3Unversioned…/Optimistic…/Numeric…sealed leaves. The hot path is the per-rowCaptureVersionvirtual override:ClosedShapeQueryOnlySelectoris unchanged (no concurrency capture path).Registration
ClosedShapeRegistration.BuildProvidergains three small helpers (BuildLightweightStorage/BuildIdentityMapStorage/BuildDirtyCheckedStorage) that switch ondescriptor.ConcurrencyModeONCE at registration time and construct the right leaf. The closed-shapeDocumentProviderkeeps its 4-tuple shape; only the writeable members change leaf identity.Public API impact
IDocumentStorage<T, TId>andDocumentStorage<T, TId>are unchanged.The three writeable storage classes flip from
public sealed→public abstract. They still exist as public types but are no longer directly instantiable — the registration internals build the concurrency-specific leaves. The W3 spike'sUseLightweightSequentialGuidClosedShape/UseExternallyAssignedStringClosedShapeextensions continue to work because they route throughClosedShapeRegistration.RegisterClosedShape.All 30 new variants are
internal sealed.Why this also helps AOT
Before: the storage class was monomorphic post-construction but the JIT couldn't prove it because every method still read
_descriptorto decide what to do. After: monomorphic by construction — the JIT (and a future Marten source generator) can specialize and inline aggressively. This is the necessary precondition for source-gen specialization ofIDocumentAccessor<TDoc, TId>per the W5 spike notes.Acceptance criteria
All new variants are
internal sealed. No new public API surface.OverwriteProjected(#4658) still routes through the Optimistic / Numeric variant appropriately and passesnulltrackers in the versioned variants.Local test results
(One unrelated
Bug_4187_ancillary_store_isolationCoreTests flake — "Unable to attain a global lock in time" — is a pre-existing timing issue unrelated to this refactor.)What this PR does NOT do
IsConjoined(4 param-binding sites) andUseVersionFromMatchingStream(rolled into Numeric leaves) — Phase 1 scope explicitly leaves the former alone per the issue's non-goals; the latter is now Numeric-only.HierarchyMapping is { }per-row branch in the 4 read selectors — that's the Phase 2 follow-up PR per the issue's phased plan.DocumentStorageDescriptorandDocumentStorageDescriptorBuilderare untouched (per the issue: "DescriptorBuilder is unchanged — it already computes everything; we just slice the registration further").Tracking
🤖 Generated with Claude Code