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]