From 2eec4368501f1739b3f5180d098de91a300aea41 Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Tue, 29 Sep 2026 10:12:20 +0200 Subject: [PATCH] Variants C1: the index rule is linear, a merge judges only when a variant is involved, duplicates are deterministic - RefuseVariantIndexSharing built its holder list per landing record by scanning every landing record, so opening a store with one variant was quadratic. It now groups what lands once and looks each (index, value) up once. Pinned by a count of index-accessor reads at two sizes, not a clock. - JudgeMerge decoded every unseen disk record before Resolve could exit early, so a plain-only store failed a conditional commit on another writer's schema it does not register. It now judges only when the batch, the disk (by the record's variant header, never its payload) or this cache holds a variant. - Plain duplicates find the holder with the smallest key (ordinal), so the choice is the same live, after a flush and after a reopen. The comment claiming "most recently indexed" was wrong across reload. - A dropped slot is reported once per record, not once by the schema diff and again by the variant. Co-Authored-By: Claude Sonnet 5 --- src/GameCult.Caching/CultCache.cs | 76 ++++++--- tests/GameCult.Caching.Tests/VariantTests.cs | 165 ++++++++++++++++++- 2 files changed, 210 insertions(+), 31 deletions(-) diff --git a/src/GameCult.Caching/CultCache.cs b/src/GameCult.Caching/CultCache.cs index 65c75b93..b2f06659 100644 --- a/src/GameCult.Caching/CultCache.cs +++ b/src/GameCult.Caching/CultCache.cs @@ -1799,7 +1799,7 @@ namespace GameCult.Caching private readonly List<(CacheBackingStore Store, Type[] Homes)> _stores = new(); private readonly Dictionary _entries = new(StringComparer.Ordinal); private readonly Dictionary> _names = new(); - // Every key holding a value, oldest first: the index is truthful about duplicates, so removing one holder never hides another. + // Every key holding a value (unordered): the index is truthful about duplicates, so removing one holder never hides another. private readonly Dictionary<(Type Type, string Alias), Dictionary>> _indexes = new(); private readonly Dictionary _globals = new(); private readonly CultCodec? _codec; @@ -1823,6 +1823,9 @@ namespace GameCult.Caching public CultDocumentRegistry Registry => _registry; + // Read under the gate, by a store judging a merge. + internal bool HoldsVariants => _variantKeys.Count > 0; + public CultCodec? Codec => _codec; // The whole override of one member, encoded with this cache's codec, ready for UpsertVariant. @@ -2124,13 +2127,16 @@ namespace GameCult.Caching .SelectMany(pair => pair.Value.TryGetValue(name, out var key) ? new[] { key } : Array.Empty())); } - // A value held by one record finds it. Records of one type sharing a value (plain duplicates) find the most recently - // indexed holder, and removing that holder finds the one before it: a held value never finds nothing. + // A value held by one record finds it. Records of one type sharing a value (plain duplicates) find the holder with the + // smallest key (ordinal), whatever order they were written or loaded in: the choice is a function of the held set, so + // it is the same live, after a flush and after a reopen, and removing that holder finds the next. public T? GetByIndex(string alias, string value) where T : class { return Single($"index {alias}='{value}'", () => _indexes .Where(pair => pair.Key.Alias == alias && typeof(T).IsAssignableFrom(pair.Key.Type)) - .SelectMany(pair => pair.Value.TryGetValue(value, out var keys) ? new[] { keys[keys.Count - 1] } : Array.Empty())); + .SelectMany(pair => pair.Value.TryGetValue(value, out var keys) + ? new[] { keys.Aggregate((best, next) => string.CompareOrdinal(next, best) < 0 ? next : best) } + : Array.Empty())); } public void Dispose() @@ -2645,6 +2651,9 @@ namespace GameCult.Caching bool IsVariant(string key) => landingByKey.TryGetValue(key, out var landed) ? landed.Variant != null : _variantKeys.Contains(key); + // One pass over what lands, one lookup per (index, value): the rule is linear in the records landing. + var landedValues = new List<(CultStoredDocument Stored, (Type Type, string Alias) Index, string Value)>(); + var landed = new Dictionary<((Type Type, string Alias) Index, string Value), List>(); foreach (var stored in landing) { foreach (var pair in stored.Descriptor.IndexAccessors) @@ -2652,25 +2661,38 @@ namespace GameCult.Caching var value = pair.Value(stored.Document); if (string.IsNullOrWhiteSpace(value)) continue; - var type = stored.Descriptor.DocumentType; - var holders = (_indexes.TryGetValue((type, pair.Key), out var held) && held.TryGetValue(value, out var heldKeys) + var index = (stored.Descriptor.DocumentType, pair.Key); + landedValues.Add((stored, index, value)); + if (!landed.TryGetValue((index, value), out var landedKeys)) + landed[(index, value)] = landedKeys = new List(); + landedKeys.Add(stored.Key.Value); + } + } + + var holdersOf = new Dictionary<((Type Type, string Alias) Index, string Value), (string[] All, string[] Variants)>(); + foreach (var (stored, index, value) in landedValues) + { + if (!holdersOf.TryGetValue((index, value), out var known)) + { + var all = (_indexes.TryGetValue(index, out var held) && held.TryGetValue(value, out var heldKeys) ? heldKeys.Where(heldKey => !replaced.Contains(heldKey)) : Enumerable.Empty()) - .Concat(landing - .Where(other => other.Descriptor.DocumentType == type && pair.Value(other.Document) == value) - .Select(other => other.Key.Value)) - .Where(holder => holder != stored.Key.Value) + .Concat(landed[(index, value)]) .Distinct(StringComparer.Ordinal) .ToArray(); - if (stored.Variant != null && holders.Length > 0) - throw new InvalidOperationException( - $"Variant {stored.Key.Value} and {holders[0]} both hold index {pair.Key} '{value}'; a variant must override its indexed member."); - var variantHolder = holders.FirstOrDefault(IsVariant); - if (stored.Variant == null && variantHolder != null) - throw new InvalidOperationException( - $"Record {stored.Key.Value} and variant {variantHolder} both hold index {pair.Key} '{value}'; " + - "a variant may not share an indexed value with another record."); + holdersOf[(index, value)] = known = (all, all.Where(IsVariant).ToArray()); } + + var self = stored.Key.Value; + var holder = known.All.FirstOrDefault(candidate => candidate != self); + if (stored.Variant != null && holder != null) + throw new InvalidOperationException( + $"Variant {self} and {holder} both hold index {index.Alias} '{value}'; a variant must override its indexed member."); + var variantHolder = known.Variants.FirstOrDefault(candidate => candidate != self); + if (stored.Variant == null && variantHolder != null) + throw new InvalidOperationException( + $"Record {self} and variant {variantHolder} both hold index {index.Alias} '{value}'; " + + "a variant may not share an indexed value with another record."); } } @@ -2795,7 +2817,6 @@ namespace GameCult.Caching RemoveHolder(pair.Value, stored.Key.Value); } - // Re-indexing a key makes it the most recent holder of its value. private static void AddHolder(Dictionary<(Type Type, string Alias), Dictionary>> indexes, (Type, string) index, string value, string key) { if (!indexes.TryGetValue(index, out var map)) @@ -2974,9 +2995,10 @@ namespace GameCult.Caching .ToArray(); if (shed.Length > 0) { - var alreadyReported = report.IgnoredExtraSlots; - report.IgnoredExtraSlots = alreadyReported.Concat(shed.Except(alreadyReported)).OrderBy(slot => slot).ToArray(); - report.Warnings = report.Warnings.Concat(shed.Select(slot => new CultSchemaMigrationWarning + // A slot the schema diff already reported is not reported twice. + var unreported = shed.Except(report.IgnoredExtraSlots).ToArray(); + report.IgnoredExtraSlots = report.IgnoredExtraSlots.Concat(unreported).OrderBy(slot => slot).ToArray(); + report.Warnings = report.Warnings.Concat(unreported.Select(slot => new CultSchemaMigrationWarning { Code = "ignored_extra_slot", Message = $"Variant {record.Key} overrides slot {slot}, which {descriptor.DocumentType.Name} does not have; the override is ignored and shed on flush." @@ -3134,7 +3156,7 @@ namespace GameCult.Caching // The committer judged the batch against its own view; the file may have moved since. A merge is judged on the // set the file will hold, exactly as that set would be judged loading: what other processes wrote (arriving), // what they removed (departing), and this batch. - if (ontoDisk && Judging != null) + if (ontoDisk && Judging != null && MergeInvolvesVariants(request, disk)) JudgeMerge(request, disk); foreach (var entry in request.Deletes) records.Remove(entry.Key.Value); @@ -3150,6 +3172,14 @@ namespace GameCult.Caching return CultCommitOutcome.Committed; } + // Only a variant makes a merge more than the committer's own judgement: a variant it lands, one another writer wrote + // (read from the record's header, never its payload), or one this cache holds. A plain-only store never decodes + // another writer's records, so a schema this cache does not register cannot fail a commit that involves no variant. + private bool MergeInvolvesVariants(CultCommitRequest request, CultPersistedStoreSnapshot disk) => + Cache?.HoldsVariants == true || + request.Upserts.Any(entry => entry.Variant != null) || + disk.Records.Any(record => record.Variant != null); + private void JudgeMerge(CultCommitRequest request, CultPersistedStoreSnapshot disk) { var landing = request.Upserts.Select(entry => entry.Key.Value).ToHashSet(StringComparer.Ordinal); diff --git a/tests/GameCult.Caching.Tests/VariantTests.cs b/tests/GameCult.Caching.Tests/VariantTests.cs index 6f6654e2..c8d7a8df 100644 --- a/tests/GameCult.Caching.Tests/VariantTests.cs +++ b/tests/GameCult.Caching.Tests/VariantTests.cs @@ -773,22 +773,22 @@ namespace GameCult.Caching.Tests })); [Test] - public void PlainDuplicatesFindTheMostRecentHolderAndRemovingEitherLeavesTheOtherFindable() + public void RemovingTheHolderADuplicateValueFindsLeavesTheOtherFindable() { var path = PathOf("gear.cc"); using (var cache = Open(path)) { var pair = SharedCodePair(cache); - Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("p2"), "the last writer"); + Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("p1"), "the smallest key"); - cache.Remove(pair[1]); - Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("p1"), "removing the winner finds the other holder"); - - cache.Commit(batch => batch.Upsert(typeof(VariantGear), Named("p2", "x"), pair[1])); - Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("p2")); cache.Remove(pair[0]); - Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("p2"), "removing the loser changes nothing"); + Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("p2"), "removing the holder finds the other one"); + + cache.Commit(batch => batch.Upsert(typeof(VariantGear), Named("p1", "x"), pair[0])); + Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("p1")); cache.Remove(pair[1]); + Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("p1"), "removing the other changes nothing"); + cache.Remove(pair[0]); Assert.That(cache.GetByIndex("code", "x"), Is.Null); cache.BackingStores[0].PushAll(); } @@ -946,6 +946,155 @@ namespace GameCult.Caching.Tests Assert.That(narrow.BackingStores[0].LastSchemaMigrationReports.SelectMany(report => report.IgnoredExtraSlots), Does.Contain(2)); } + // ---- batch 4: linear rule, judge only when a variant is involved, deterministic duplicates, one warning ---- + + [CultDocument("tests.variant_counted", "tests.variant_counted.v1")] + [MessagePackObject(AllowPrivate = true)] + internal sealed class CountedGear + { + public static long CodeReads; + [IgnoreMember] + private string _code = string.Empty; + + [Key(0)] + [CultName] + public string Name = string.Empty; + + [Key(1)] + [CultIndex("code")] + public string Code + { + get + { + System.Threading.Interlocked.Increment(ref CodeReads); + return _code; + } + set => _code = value; + } + } + + // The pin is a count of index-accessor reads, not a clock: linear work doubles with the records, quadratic work quadruples. + private long IndexReadsOpening(int records) + { + var registry = CultDocumentRegistry.ForTypes(new[] { typeof(CountedGear) }); + var path = PathOf($"counted-{records}.cc"); + using (var writer = Open(path, registry: registry)) + { + writer.Commit(batch => + { + for (var index = 0; index < records; index++) + batch.Upsert(typeof(CountedGear), new CountedGear { Name = $"n{index}", Code = $"c{index}" }, new CultRecordKey($"k{index}")); + }); + writer.Commit(batch => batch.UpsertVariant(new CultRecordKey("kv"), new CultRecordKey("k0"), new[] + { + writer.Override(nameof(CountedGear.Name), "variant"), + writer.Override(nameof(CountedGear.Code), "variant-code") + })); + } + + System.Threading.Interlocked.Exchange(ref CountedGear.CodeReads, 0); + using (Open(path, registry: registry)) + return System.Threading.Interlocked.Read(ref CountedGear.CodeReads); + } + + [Test] + public void OpeningAStoreWithAVariantReadsEachIndexedValueAFixedNumberOfTimes() + { + var small = IndexReadsOpening(1000); + var large = IndexReadsOpening(2000); + Assert.That(large, Is.LessThan(small * 3), $"index reads: {small} at 1000 records, {large} at 2000; doubling the records must not quadruple the work"); + } + + [Test] + public void ASchemaThisCacheDoesNotRegisterDoesNotFailAConditionalCommitThatInvolvesNoVariant() + { + var path = PathOf("gear.cc"); + var other = new CultRecordKey("other"); + var narrow = CultDocumentRegistry.ForTypes(new[] { typeof(VariantGear) }); + using (var seed = Open(path)) + SeedBase(seed); + + using (var a = Open(path)) + using (var b = Open(path, registry: narrow)) + { + a.Commit(batch => batch.Upsert(typeof(VariantOther), new VariantOther { Name = "foreign" }, new CultRecordKey("foreign"))); + Assert.That(b.Commit(batch => + { + batch.Expect(other, null); + batch.Upsert(typeof(VariantGear), Named("other", "fine"), other); + }), Is.True); + } + + using var reopened = Open(path); + Assert.That(reopened.Get(other), Is.Not.Null); + Assert.That(reopened.Get(new CultRecordKey("foreign")), Is.Not.Null, "the other writer's record survives the merge"); + } + + [Test] + public void AVariantAnotherWriterLandedIsStillJudgedWhenThisCacheHoldsNoVariant() + { + var path = PathOf("gear.cc"); + var other = new CultRecordKey("other"); + using (var seed = Open(path)) + SeedBase(seed); + + using (var a = Open(path)) + using (var b = Open(path)) + { + VariantOnCode(a, "shared"); + Refused(() => b.Commit(batch => + { + batch.Expect(other, null); + batch.Upsert(typeof(VariantGear), Named("other", "shared"), other); + })); + } + } + + [Test] + public void PlainDuplicatesFindTheSmallestKeyLiveAfterAFlushAndAfterAReopen() + { + var path = PathOf("gear.cc"); + using (var cache = Open(path)) + { + cache.Commit(batch => batch.Upsert(typeof(VariantGear), Named("nb", "x"), new CultRecordKey("b"))); + cache.Commit(batch => batch.Upsert(typeof(VariantGear), Named("na", "x"), new CultRecordKey("a"))); + Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("na"), "live"); + cache.BackingStores[0].PushAll(); + Assert.That(cache.GetByIndex("code", "x")!.Name, Is.EqualTo("na"), "after a flush"); + } + + using (var reopened = Open(path)) + { + Assert.That(reopened.GetByIndex("code", "x")!.Name, Is.EqualTo("na"), "after a reopen"); + reopened.Remove(new CultRecordKey("a")); + reopened.Commit(batch => batch.Upsert(typeof(VariantGear), Named("na", "x"), new CultRecordKey("a"))); + Assert.That(reopened.GetByIndex("code", "x")!.Name, Is.EqualTo("na"), "rewriting the smaller key does not change the choice"); + } + } + + [Test] + public void ADroppedSlotIsReportedOncePerRecord() + { + var path = PathOf("drift.cc"); + using (var writer = Open(path, registry: CultDocumentRegistry.ForTypes(new[] { typeof(DriftWide) }))) + { + writer.Commit(batch => + { + batch.Upsert(typeof(DriftWide), new DriftWide { Name = "drift", Power = 1, Wing = "base wing" }, new CultRecordKey("drift-base")); + batch.UpsertVariant(new CultRecordKey("drift-variant"), new CultRecordKey("drift-base"), new[] + { + writer.Override(nameof(DriftWide.Name), "drift variant"), + writer.Override(nameof(DriftWide.Wing), "stale wing") + }); + }); + } + + using var narrow = Open(path, registry: CultDocumentRegistry.ForTypes(new[] { typeof(DriftNarrow) })); + var variantReport = narrow.BackingStores[0].LastSchemaMigrationReports.Last(); + Assert.That(variantReport.Warnings.Count(warning => warning.Code == "ignored_extra_slot"), Is.EqualTo(1)); + Assert.That(variantReport.IgnoredExtraSlots, Is.EqualTo(new[] { 2 })); + } + // ---- the directory store refuses a variant page on load as it does on write ---- [Test]