From bdf362c330c3a2c4637a00c47bd61f28821899d5 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 25 Sep 2026 18:45:02 -0400 Subject: [PATCH 01/16] refactor: remove analysis cascade memo Co-Authored-By: Claude Opus 5.5 --- .../AnalysisScope.cs | 134 --------------- .../AnalysisStateKey.cs | 133 --------------- .../AnalysisStratumRule.cs | 156 ++++++++++++------ .../MemoizedCombinationRuleCascade.cs | 118 ------------- .../Morpher.cs | 47 +----- src/SIL.Machine.Morphology.HermitCrab/Word.cs | 98 +---------- 6 files changed, 115 insertions(+), 571 deletions(-) delete mode 100644 src/SIL.Machine.Morphology.HermitCrab/AnalysisScope.cs delete mode 100644 src/SIL.Machine.Morphology.HermitCrab/AnalysisStateKey.cs delete mode 100644 src/SIL.Machine.Morphology.HermitCrab/MemoizedCombinationRuleCascade.cs diff --git a/src/SIL.Machine.Morphology.HermitCrab/AnalysisScope.cs b/src/SIL.Machine.Morphology.HermitCrab/AnalysisScope.cs deleted file mode 100644 index 19504f2f0..000000000 --- a/src/SIL.Machine.Morphology.HermitCrab/AnalysisScope.cs +++ /dev/null @@ -1,134 +0,0 @@ -using System.Collections.Generic; - -namespace SIL.Machine.Morphology.HermitCrab -{ - /// - /// Carrier for the analysis-cascade memo, threaded through clones - /// like and likewise excluded from Word.FreezeImpl/ - /// Word.ValueEquals, so dedup semantics are unchanged. - /// - /// One instance per call. A state key does not - /// encode the target surface word, so sharing a scope across parses of different words would be - /// unsound. - /// - /// - /// Not thread-safe, hence the plain collections: a scope is only installed when - /// is 1. Memoizing the parallel cascade would require - /// concurrent ones. - /// - /// - internal sealed class AnalysisScope - { - // OOM guards; past either cap subtrees simply go unmemoized, degrading hit rate but never - // correctness. The Word budget is the load-bearing one: entry size is unbounded (a node's list - // holds every descendant, undeduplicated) and storing them keeps every intermediate of the search - // alive for the whole parse. Both tables share it. It is a coarse backstop, not a figure derived - // from measured memory. - private const int MaxMemoEntries = 100_000; - private const int MaxMemoWords = 1_000_000; - - private int _storedWordCount; - - public Dictionary Memo { get; } = new Dictionary(); - - // Same key space as Memo, different computation: the affix-template battery's result for a state - // (AnalysisStratumRule.ApplyTemplateBattery). Separate because a state can be memoized in one - // table but not the other. - public Dictionary TemplateMemo { get; } = - new Dictionary(); - - // Keys still under expansion on the call stack; a re-arrival at one must fall through to - // unmemoized expansion rather than read a partial entry. Defensive only -- no path reaches it - // today, since every unapplication grows the multiset the key hashes, so a key cannot recur while - // still on the stack. The template battery needs no equivalent: its call is eager. - public HashSet InProgress { get; } = new HashSet(); - - // Per-parse hit counts, folded into the owning Morpher when the parse ends. Equivalence tests - // assert on them: a memo that silently stopped firing looks exactly like a passing test. - public int MemoHits { get; set; } - public int NogoodHits { get; set; } - public int TemplateMemoHits { get; set; } - public int TemplateNogoodHits { get; set; } - - /// - /// Replay shared by both memo consumers. False on a miss; on a hit - /// holds the stored results grafted onto , or - /// is empty for a stored-empty ("nogood") entry. The query's non-head prefix is cloned once and - /// shared across this hit's replays, which is safe because each replay freezes immediately and - /// every non-head mutation path is CheckFrozen-guarded. - /// - public bool TryReplay( - Dictionary table, - AnalysisStateKey key, - Word query, - out List replayed - ) - { - if (!table.TryGetValue(key, out MemoEntry entry)) - { - replayed = null; - return false; - } - if (entry.Results.Count == 0) - { - replayed = new List(); - return true; - } - List queryNonHeadPrefix = query.CloneNonHeadsForReplay(); - replayed = new List(entry.Results.Count); - foreach (Word stored in entry.Results) - { - replayed.Add( - stored.ReplayOnto( - query, - entry.MruleTrailPrefixLength, - entry.NonHeadPrefixLength, - queryNonHeadPrefix - ) - ); - } - return true; - } - - /// - /// Records a fully-expanded result list against , unless either the table is - /// full or the parse's retained-Word budget cannot absorb it. - /// - public void Store( - Dictionary table, - AnalysisStateKey key, - Word query, - List results - ) - { - if (table.Count >= MaxMemoEntries || _storedWordCount > MaxMemoWords - results.Count) - return; - _storedWordCount += results.Count; - table[key] = new MemoEntry(results, query.MorphologicalRuleTrailLength, query.NonHeadCount); - } - } - - /// - /// A memoized subtree or template-battery result. An empty means the state was - /// proved to yield nothing. The two prefix lengths are the trail/non-head counts at the moment of the - /// write, which is where splits a stored result when grafting it onto a - /// new arrival. - /// - /// There is deliberately no "incomplete" flag: only fully-explored subtrees may be recorded. Should a - /// step or time budget ever be added, an interrupted subtree must not be stored. - /// - /// - internal sealed class MemoEntry - { - public MemoEntry(IReadOnlyList results, int mruleTrailPrefixLength, int nonHeadPrefixLength) - { - Results = results; - MruleTrailPrefixLength = mruleTrailPrefixLength; - NonHeadPrefixLength = nonHeadPrefixLength; - } - - public IReadOnlyList Results { get; } - public int MruleTrailPrefixLength { get; } - public int NonHeadPrefixLength { get; } - } -} diff --git a/src/SIL.Machine.Morphology.HermitCrab/AnalysisStateKey.cs b/src/SIL.Machine.Morphology.HermitCrab/AnalysisStateKey.cs deleted file mode 100644 index d406cbe3f..000000000 --- a/src/SIL.Machine.Morphology.HermitCrab/AnalysisStateKey.cs +++ /dev/null @@ -1,133 +0,0 @@ -using System; -using System.Collections.Generic; -using SIL.Machine.Annotations; -using SIL.Machine.FeatureModel; - -namespace SIL.Machine.Morphology.HermitCrab -{ - /// - /// Order-independent identity of an analysis-cascade node. Two Words with an equal - /// key must make identical decisions in every analysis-side rule the cascade can invoke; that is the - /// memo's correctness contract, so this key-completeness audit of what each rule reads has to be - /// re-run whenever an Analysis*.cs rule changes: - /// - /// : Shape (FST pattern match), - /// (unifiability gate), per-rule unapplication count - /// and FinalTemplateState. - /// : adds - /// (MaxStemCount gate) -- never the non-heads' own content, only the count - /// and FinalTemplateState. - /// : adds - /// . - /// - /// No rule reads the order those rules were unapplied in, which is the redundancy this key collapses, - /// so the trail is reduced to an unordered multiset here. _isLastAppliedRuleFinal is excluded - /// as well: Word.ValueEquals includes it for result dedup, but no analysis-side rule reads it. - /// - /// AnalysisStratumRule also merges equivalent analyses under this key. A merged alternative gets - /// no rule applications of its own, so that use needs the same completeness. - /// - /// - internal readonly struct AnalysisStateKey : IEquatable - { - private readonly Shape _shape; - private readonly Stratum _stratum; - private readonly FeatureStruct _syntacticFS; - private readonly FeatureStruct _realizationalFS; - private readonly int _nonHeadCount; - private readonly IReadOnlyDictionary _ruleCounts; - private readonly FinalTemplateState _finalTemplateState; - private readonly int _hashCode; - - /// - /// Keys , freezing the fields the key reads. A named factory because that - /// freeze mutates : the cached hash calls GetFrozenHashCode, and - /// Word.FreezeImpl leaves SyntacticFeatureStruct unfrozen. - /// - public static AnalysisStateKey PinAndKey(Word word) - { - return new AnalysisStateKey(word); - } - - private AnalysisStateKey(Word word) - { - // The cached hash covers live references -- notably Word.UnappliedRuleCounts, the word's own - // mutable dictionary. Keying an unfrozen word would let a later mutation invalidate a stored - // key's hash, silently causing permanent misses or entries that no longer match their bucket. - if (!word.IsFrozen) - throw new ArgumentException( - "The word must be frozen before it can be used as a memo key.", - nameof(word) - ); - - _shape = word.Shape; - _stratum = word.Stratum; - _syntacticFS = word.SyntacticFeatureStruct; - _realizationalFS = word.RealizationalFeatureStruct; - _nonHeadCount = word.NonHeadCount; - _ruleCounts = word.UnappliedRuleCounts; - _finalTemplateState = word.FinalTemplateState; - - // See PinAndKey for why the key pins these rather than just reading them. - _shape.Freeze(); - _syntacticFS.Freeze(); - _realizationalFS.Freeze(); - - int hash = 17; - hash = hash * 31 + _shape.GetFrozenHashCode(); - hash = hash * 31 + (_stratum?.GetHashCode() ?? 0); - hash = hash * 31 + _syntacticFS.GetFrozenHashCode(); - hash = hash * 31 + _realizationalFS.GetFrozenHashCode(); - hash = hash * 31 + _nonHeadCount; - hash = hash * 31 + _finalTemplateState.GetHashCode(); - if (_ruleCounts != null) - { - // XOR rather than the usual *31 rolling combine: the multiset is unordered, so entries - // accumulated in different unapplication orders must still hash identically. - int multisetHash = 0; - foreach (KeyValuePair kvp in _ruleCounts) - multisetHash ^= (kvp.Key.GetHashCode() * 397) ^ kvp.Value; - hash = hash * 31 + multisetHash; - } - _hashCode = hash; - } - - public override int GetHashCode() => _hashCode; - - public override bool Equals(object obj) => obj is AnalysisStateKey other && Equals(other); - - public bool Equals(AnalysisStateKey other) - { - if (_hashCode != other._hashCode) - return false; - if (_nonHeadCount != other._nonHeadCount || !ReferenceEquals(_stratum, other._stratum)) - return false; - if (!_shape.ValueEquals(other._shape)) - return false; - if (!_syntacticFS.ValueEquals(other._syntacticFS) || !_realizationalFS.ValueEquals(other._realizationalFS)) - return false; - if (_finalTemplateState != other._finalTemplateState) - return false; - return RuleCountsEqual(_ruleCounts, other._ruleCounts); - } - - private static bool RuleCountsEqual( - IReadOnlyDictionary a, - IReadOnlyDictionary b - ) - { - int aCount = a?.Count ?? 0; - int bCount = b?.Count ?? 0; - if (aCount != bCount) - return false; - if (aCount == 0) - return true; - foreach (KeyValuePair kvp in a) - { - if (!b.TryGetValue(kvp.Key, out int otherCount) || otherCount != kvp.Value) - return false; - } - return true; - } - } -} diff --git a/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs b/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs index 7be4e1cad..081a686e4 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.Linq; using SIL.Machine.Annotations; +using SIL.Machine.FeatureModel; using SIL.Machine.Rules; using SIL.ObjectModel; @@ -47,18 +48,25 @@ public AnalysisStratumRule(Morpher morpher, Stratum stratum) ); break; case MorphologicalRuleOrder.Unordered: - _mrulesRule = - morpher.MaxDegreeOfParallelism == 1 - ? (RuleCascade) - new MemoizedCombinationRuleCascade(mrules, FreezableEqualityComparer.Default) - : new ParallelCombinationRuleCascade( - mrules, - true, - FreezableEqualityComparer.Default - ) - { - MaxDegreeOfParallelism = morpher.MaxDegreeOfParallelism, - }; + if (morpher.MaxDegreeOfParallelism == 1) + { + _mrulesRule = new CombinationRuleCascade( + mrules, + true, + FreezableEqualityComparer.Default + ); + } + else + { + _mrulesRule = new ParallelCombinationRuleCascade( + mrules, + true, + FreezableEqualityComparer.Default + ) + { + MaxDegreeOfParallelism = morpher.MaxDegreeOfParallelism, + }; + } break; } } @@ -129,11 +137,11 @@ internal IEnumerable Apply(Word input, ref int alternativeCount) _prulesRule.Apply(input); input.Freeze(); - IDictionary wordCache = null; + IDictionary wordCache = null; // Don't merge if tracing because it messes up the tracing. bool mergeEquivalentAnalyses = _morpher.MergeEquivalentAnalyses && !_morpher.TraceManager.IsTracing; if (mergeEquivalentAnalyses) - wordCache = new Dictionary(); + wordCache = new Dictionary(); // AnalysisStratumRule.Apply should cover the inverse of SynthesisStratumRule.Apply. IEnumerable mruleOutWords = ApplyTemplates(input).Concat(ApplyMorphologicalRules(input)); @@ -152,10 +160,10 @@ internal IEnumerable Apply(Word input, ref int alternativeCount) } // Skip intermediate sources from phonological rules, templates, and morphological rules. mruleOutWord.Source = origInput; - AnalysisStateKey key = default; + AnalysisMergeKey key = default; if (mergeEquivalentAnalyses) { - key = AnalysisStateKey.PinAndKey(mruleOutWord); + key = new AnalysisMergeKey(mruleOutWord); if (wordCache.TryGetValue(key, out Word canonicalWord)) { canonicalWord.Alternatives.Add(mruleOutWord); @@ -193,38 +201,9 @@ private IEnumerable ApplyMorphologicalRules(Word input) } } - // The affix-template battery, memoized by AnalysisStateKey against its own table. On - // template-heavy grammars this dominates parse time, which is why it is memoized separately from - // the mrule cascade. See AnalysisScope.InProgress for why no re-entry guard is needed here. - private IEnumerable ApplyTemplateBattery(Word input) - { - // Scope presence is the single source of truth for whether the memo is active; Morpher decides - // that once, at install time. Linear strata stay unmemoized because the key-completeness audit - // covers only the Unordered cascade. - AnalysisScope scope = input.AnalysisScope; - if (scope == null || _stratum.MorphologicalRuleOrder != MorphologicalRuleOrder.Unordered) - return _templatesRule.Apply(input); - - AnalysisStateKey key = AnalysisStateKey.PinAndKey(input); - if (scope.TryReplay(scope.TemplateMemo, key, input, out List replayed)) - { - if (replayed.Count == 0) - { - scope.TemplateNogoodHits++; - return replayed; - } - scope.TemplateMemoHits++; - return replayed; - } - - var results = new List(_templatesRule.Apply(input)); - scope.Store(scope.TemplateMemo, key, input, results); - return results; - } - private IEnumerable ApplyTemplates(Word input) { - foreach (Word tempOutWord in ApplyTemplateBattery(input).Distinct(FreezableEqualityComparer.Default)) + foreach (Word tempOutWord in _templatesRule.Apply(input).Distinct(FreezableEqualityComparer.Default)) { switch (_stratum.MorphologicalRuleOrder) { @@ -246,5 +225,90 @@ private IEnumerable ApplyTemplates(Word input) } } } + + internal readonly struct AnalysisMergeKey : IEquatable + { + private readonly Shape _shape; + private readonly Stratum _stratum; + private readonly FeatureStruct _syntacticFS; + private readonly FeatureStruct _realizationalFS; + private readonly int _nonHeadCount; + private readonly IReadOnlyDictionary _ruleCounts; + private readonly int _hashCode; + + public AnalysisMergeKey(Word word) + { + if (!word.IsFrozen) + throw new ArgumentException( + "The word must be frozen before equivalent analyses can be merged.", + nameof(word) + ); + + _shape = word.Shape; + _stratum = word.Stratum; + _syntacticFS = word.SyntacticFeatureStruct; + _realizationalFS = word.RealizationalFeatureStruct; + _nonHeadCount = word.NonHeadCount; + _ruleCounts = word.UnappliedRuleCounts; + + _shape.Freeze(); + _syntacticFS.Freeze(); + _realizationalFS.Freeze(); + + int hash = 17; + hash = hash * 31 + _shape.GetFrozenHashCode(); + hash = hash * 31 + (_stratum?.GetHashCode() ?? 0); + hash = hash * 31 + _syntacticFS.GetFrozenHashCode(); + hash = hash * 31 + _realizationalFS.GetFrozenHashCode(); + hash = hash * 31 + _nonHeadCount; + if (_ruleCounts != null) + { + int multisetHash = 0; + foreach (KeyValuePair kvp in _ruleCounts) + multisetHash ^= (kvp.Key.GetHashCode() * 397) ^ kvp.Value; + hash = hash * 31 + multisetHash; + } + _hashCode = hash; + } + + public override int GetHashCode() => _hashCode; + + public override bool Equals(object obj) => obj is AnalysisMergeKey other && Equals(other); + + public bool Equals(AnalysisMergeKey other) + { + if (_hashCode != other._hashCode) + return false; + if (_nonHeadCount != other._nonHeadCount || !ReferenceEquals(_stratum, other._stratum)) + return false; + if (!_shape.ValueEquals(other._shape)) + return false; + if ( + !_syntacticFS.ValueEquals(other._syntacticFS) + || !_realizationalFS.ValueEquals(other._realizationalFS) + ) + return false; + return RuleCountsEqual(_ruleCounts, other._ruleCounts); + } + + private static bool RuleCountsEqual( + IReadOnlyDictionary a, + IReadOnlyDictionary b + ) + { + int aCount = a?.Count ?? 0; + int bCount = b?.Count ?? 0; + if (aCount != bCount) + return false; + if (aCount == 0) + return true; + foreach (KeyValuePair kvp in a) + { + if (!b.TryGetValue(kvp.Key, out int otherCount) || otherCount != kvp.Value) + return false; + } + return true; + } + } } } diff --git a/src/SIL.Machine.Morphology.HermitCrab/MemoizedCombinationRuleCascade.cs b/src/SIL.Machine.Morphology.HermitCrab/MemoizedCombinationRuleCascade.cs deleted file mode 100644 index 6e1bc1e72..000000000 --- a/src/SIL.Machine.Morphology.HermitCrab/MemoizedCombinationRuleCascade.cs +++ /dev/null @@ -1,118 +0,0 @@ -using System.Collections.Generic; -using SIL.Machine.Annotations; -using SIL.Machine.Rules; - -namespace SIL.Machine.Morphology.HermitCrab -{ - /// - /// The sequential plus memoization of each - /// expanded subtree, for Unordered-order analysis strata. A node whose - /// was already searched earlier in this word's analysis, via a - /// different unapplication order, is not searched again: an empty stored result short-circuits, and a - /// non-empty one is replayed onto the current arrival (). - /// - /// The parallel cascade is left unmemoized -- its breadth-first walk never reaches a point where a - /// given subtree is known to be fully expanded, so there is nowhere to hang a memo write. - /// - /// - internal class MemoizedCombinationRuleCascade : CombinationRuleCascade - { - public MemoizedCombinationRuleCascade( - IEnumerable> rules, - IEqualityComparer comparer - ) - : base(rules, true, comparer) { } - - public override IEnumerable Apply(Word input) - { - var output = new HashSet(Comparer); - // Word.Clone carries the scope, so a null scope at the root means a null scope throughout. - if (input.AnalysisScope == null) - ApplyRulesUnmemoized(input, output); - else - ApplyRules(input, output); - return output; - } - - // Returns the results produced strictly within `input`'s subtree, at any depth, excluding `input` - // itself -- both what callers consume and what gets memoized against `input`'s key. - private List ApplyRules(Word input, HashSet output) - { - AnalysisScope scope = input.AnalysisScope; - AnalysisStateKey key = AnalysisStateKey.PinAndKey(input); - - if (scope.TryReplay(scope.Memo, key, input, out List replayed)) - { - if (replayed.Count == 0) - { - scope.NogoodHits++; - return replayed; - } - foreach (Word replay in replayed) - { - output.Add(replay); - CheckMaxAlternatives(output.Count); - } - scope.MemoHits++; - return replayed; - } - - // In-flight re-entry guard, see AnalysisScope.InProgress. - if (!scope.InProgress.Add(key)) - return ApplyRulesRaw(input, output); - - List results; - try - { - results = ApplyRulesRaw(input, output); - } - finally - { - scope.InProgress.Remove(key); - } - - scope.Store(scope.Memo, key, input, results); - return results; - } - - // Mirrors the base's multiApp expansion, including its recurse-before-add ordering. Delegating to - // the base is not possible -- it collects into one globally-deduped set, so a subtree result another - // branch already contributed is missing from it, yet must still be recorded here or a later replay - // of this key would return too few results. - private List ApplyRulesRaw(Word input, HashSet output) - { - var local = new List(); - for (int i = 0; i < Rules.Count; i++) - { - foreach (Word result in ApplyRule(Rules[i], i, input)) - { - // avoid infinite loop -- same guard CombinationRuleCascade uses - if (!Comparer.Equals(input, result)) - local.AddRange(ApplyRules(result, output)); - local.Add(result); - output.Add(result); - CheckMaxAlternatives(output.Count); - } - } - return local; - } - - // ApplyRulesRaw without the per-node result lists, which exist only to feed a memo write. With no - // scope there is nothing to write, and accumulating them anyway would cost allocations - // proportional to the sum of all subtree sizes. - private void ApplyRulesUnmemoized(Word input, HashSet output) - { - for (int i = 0; i < Rules.Count; i++) - { - foreach (Word result in ApplyRule(Rules[i], i, input)) - { - // avoid infinite loop -- same guard CombinationRuleCascade uses - if (!Comparer.Equals(input, result)) - ApplyRulesUnmemoized(result, output); - output.Add(result); - CheckMaxAlternatives(output.Count); - } - } - } - } -} diff --git a/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs b/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs index c48d327fe..864581961 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs @@ -25,10 +25,6 @@ public class Morpher : IMorphologicalAnalyzer, IMorphologicalGenerator private readonly ITraceManager _traceManager; private readonly ReadOnlyObservableCollection _morphemes; private readonly IList _lexicalPatterns = new List(); - private long _memoHits; - private long _nogoodHits; - private long _templateMemoHits; - private long _templateNogoodHits; public Morpher(ITraceManager traceManager, Language lang, int maxDegreeOfParallelism = 0) { @@ -96,7 +92,7 @@ public ITraceManager TraceManager public int MaxAlternatives { get; set; } /// - /// Merge analyses that are equivalent for every analysis-side rule (see ). + /// Merge analyses with equivalent state for every analysis-side rule. /// Merged analyses will be expanded if lexical lookup succeeds. /// public bool MergeEquivalentAnalyses { get; set; } @@ -119,15 +115,9 @@ public ITraceManager TraceManager public bool PruneDisagreeingCopies { get; set; } /// - /// Caps the concurrency used within a single parse or generation -- analysis cascade, - /// affix-template unapplication and synthesis alike. A value of 1 runs the work fully - /// sequentially, and is the only configuration eligible for the analysis-cascade memo (see - /// ). The default of 0, like any value below 1, leaves - /// concurrency unbounded. Constructor-only, because it determines how the analysis rules compile. - /// - /// This must remain a pure performance knob: nothing that changes which analyses a parse returns - /// may be gated on it, or the memoized and unmemoized configurations stop being comparable. - /// + /// Caps concurrency within a parse or generation, including the analysis cascade, affix-template + /// unapplication, and synthesis. A value of 1 runs the work sequentially; values below 1 leave + /// concurrency unbounded. This is constructor-only because it determines how the analysis rules compile. /// public int MaxDegreeOfParallelism { get; } @@ -146,24 +136,6 @@ internal ParallelOptions CreateParallelOptions(int uncappedDegree = -1) }; } - /// - /// Memo-hit totals over the parses this Morpher has completed; a parse that throws discards its - /// counts. Per-Morpher rather than per-process so one test's counts cannot leak into another's. - /// - internal long MemoHits => Interlocked.Read(ref _memoHits); - internal long NogoodHits => Interlocked.Read(ref _nogoodHits); - internal long TemplateMemoHits => Interlocked.Read(ref _templateMemoHits); - internal long TemplateNogoodHits => Interlocked.Read(ref _templateNogoodHits); - - // Interlocked because one Morpher may be parsing on several threads at once. - private void AccumulateMemoDiagnostics(AnalysisScope scope) - { - Interlocked.Add(ref _memoHits, scope.MemoHits); - Interlocked.Add(ref _nogoodHits, scope.NogoodHits); - Interlocked.Add(ref _templateMemoHits, scope.TemplateMemoHits); - Interlocked.Add(ref _templateNogoodHits, scope.TemplateNogoodHits); - } - public Func LexEntrySelector { get; set; } public Func RuleSelector { get; set; } @@ -195,10 +167,6 @@ public IEnumerable ParseWord(string word, out object trace, bool guessRoot Shape shape = _lang.SurfaceStratum.CharacterDefinitionTable.Segment(word); var input = new Word(_lang.SurfaceStratum, shape); - // Installing a scope is what enables the memo. Never while tracing: traces must stay - // byte-identical to the unmemoized engine. - AnalysisScope scope = !_traceManager.IsTracing && MaxDegreeOfParallelism == 1 ? new AnalysisScope() : null; - input.AnalysisScope = scope; input.Freeze(); if (_traceManager.IsTracing) _traceManager.AnalyzeWord(_lang, input); @@ -206,8 +174,6 @@ public IEnumerable ParseWord(string word, out object trace, bool guessRoot // Unapply rules var analyses = new ConcurrentQueue(_analysisRule.Apply(input)); - if (scope != null) - AccumulateMemoDiagnostics(scope); #if OUTPUT_ANALYSES var lines = new List(); @@ -465,9 +431,6 @@ LexEntry entry in SearchRootAllomorphs(input.Stratum, input.Shape) foreach (RootAllomorph allomorph in entry.Allomorphs) { Word newWord = input.Clone(); - // Synthesis never reads the memo, and keeping the reference would pin both tables for - // as long as the caller holds the returned words. - newWord.AnalysisScope = null; newWord.RootAllomorph = allomorph; if (_traceManager.IsTracing) _traceManager.SynthesizeWord(_lang, newWord); @@ -542,8 +505,6 @@ private IEnumerable LexicalGuess(Word input) } // Create a new word that uses the root allomorph. Word newWord = input.Clone(); - // Synthesis never reads the memo; see LexicalLookup. - newWord.AnalysisScope = null; newWord.RootAllomorph = root; if (_traceManager.IsTracing) _traceManager.SynthesizeWord(_lang, newWord); diff --git a/src/SIL.Machine.Morphology.HermitCrab/Word.cs b/src/SIL.Machine.Morphology.HermitCrab/Word.cs index 45d2642e6..fcff87402 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/Word.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/Word.cs @@ -73,11 +73,6 @@ public Word(Stratum stratum, Shape shape) } protected Word(Word word) - : this(word, cloneNonHeadApps: true) { } - - // ReplayOnto passes false: it rebuilds the non-head list wholesale, so cloning it here would be - // discarded work. - private Word(Word word, bool cloneNonHeadApps) { _allomorphs = new Dictionary(word._allomorphs); Stratum = word.Stratum; @@ -92,14 +87,13 @@ private Word(Word word, bool cloneNonHeadApps) _mruleAppIndex = word._mruleAppIndex; _mrulesUnapplied = new Dictionary(word._mrulesUnapplied); _mrulesApplied = new Dictionary(word._mrulesApplied); - _nonHeadApps = cloneNonHeadApps ? new List(word._nonHeadApps.CloneItems()) : new List(); + _nonHeadApps = new List(word._nonHeadApps.CloneItems()); _nonHeadAppIndex = word._nonHeadAppIndex; _obligatorySyntacticFeatures = new IDBearerSet(word._obligatorySyntacticFeatures); _isLastAppliedRuleFinal = word._isLastAppliedRuleFinal; _finalTemplateState = word._finalTemplateState; _isPartial = word._isPartial; CurrentTrace = word.CurrentTrace; - AnalysisScope = word.AnalysisScope; _disjunctiveAllomorphIndices = word._disjunctiveAllomorphIndices.ToDictionary( kvp => kvp.Key, kvp => new HashSet(kvp.Value) @@ -222,16 +216,6 @@ public IEnumerable MorphemesInApplicationOrder public object CurrentTrace { get; set; } - /// - /// Carrier for the analysis-cascade memo. Reference-shared like - /// and excluded from FreezeImpl/ValueEquals for the same - /// reason. Null while tracing, and for words not routed through - /// at all, so readers must fall back to - /// unmemoized behavior rather than throw. Cleared again on entry to synthesis so returned words do - /// not pin the per-parse tables. - /// - internal AnalysisScope AnalysisScope { get; set; } - public bool IsPartial { get { return _isPartial; } @@ -333,12 +317,6 @@ internal void RemoveMorph(Annotation morphAnn) /// indicates that an unknown compounding rule was unapplied. This is used when /// generating a compound word, because the compounding rule is usually not known just /// the non-head allomorph. - /// - /// The trail push and the count increment below must stay in lockstep: - /// splits a stored result on the assumption that equal unapplication multisets imply equal trail - /// lengths. Realizational rules incrementing the count without extending the trail is safe because - /// they do so on both sides of any comparison; any other divergence misaligns the graft. - /// /// internal void MorphologicalRuleUnapplied(IMorphologicalRule mrule) { @@ -364,9 +342,6 @@ internal int GetUnapplicationCount(IMorphologicalRule mrule) return numUnapplies; } - /// - /// The full multiset backing , for . - /// internal IReadOnlyDictionary UnappliedRuleCounts => _mrulesUnapplied; /// @@ -433,15 +408,6 @@ internal int NonHeadCount get { return _nonHeadApps.Count; } } - internal IReadOnlyList NonHeads => _nonHeadApps; - - /// - /// Length of the morphological-rule trail so far. Recorded with when a - /// subtree is memoized, to mark where a replayed result's kept suffix begins; see - /// . - /// - internal int MorphologicalRuleTrailLength => _mruleApps.Count; - internal void NonHeadUnapplied(Word nonHead) { CheckFrozen(); @@ -500,68 +466,6 @@ internal IList ExpandAlternatives() return alternatives; } - /// - /// Re-parents this Word -- computed while exploring the subtree below some cascade node N -- onto - /// , which reached N's via a different - /// unapplication order. - /// - /// Sound because an equal key means N and agree on Shape, both - /// FeatureStructs, the unapplication multiset and the non-head count, so everything computed - /// inside the subtree is a function of state they share and carries over untouched. Only the two - /// ordered structures the key reduces to counts -- the rule trail and the non-head list -- can - /// differ, and only in the prefix accumulated before reaching N, which is what gets replaced. - /// - /// - /// The word that hit the memo; its trail and non-heads become the prefix. - /// - /// N's _mruleApps.Count when its subtree was memoized: this word's trail from that index on - /// is the subtree-local suffix to keep. - /// - /// Same, for _nonHeadApps. - /// - /// Pre-cloned non-heads from , so one memo hit clones them once rather - /// than per stored result; see AnalysisScope.TryReplay. Null clones them here instead. - /// - internal Word ReplayOnto( - Word queryNode, - int mruleTrailPrefixLength, - int nonHeadPrefixLength, - IReadOnlyList queryNonHeadPrefix = null - ) - { - var clone = new Word(this, cloneNonHeadApps: false); - - List mruleSuffix = clone._mruleApps.GetRange( - mruleTrailPrefixLength, - clone._mruleApps.Count - mruleTrailPrefixLength - ); - clone._mruleApps.Clear(); - clone._mruleApps.AddRange(queryNode._mruleApps); - clone._mruleApps.AddRange(mruleSuffix); - clone._mruleAppIndex = clone._mruleApps.Count - 1; - - // The clone's non-head list starts empty, so it is built as query prefix + this word's - // subtree-local suffix without ever cloning the prefix this word arrived with, which the graft - // discards anyway. - if (queryNonHeadPrefix != null) - clone._nonHeadApps.AddRange(queryNonHeadPrefix); - else - clone._nonHeadApps.AddRange(queryNode._nonHeadApps.CloneItems()); - clone._nonHeadApps.AddRange( - _nonHeadApps.GetRange(nonHeadPrefixLength, _nonHeadApps.Count - nonHeadPrefixLength).CloneItems() - ); - clone._nonHeadAppIndex = clone._nonHeadApps.Count - 1; - - clone.Freeze(); - return clone; - } - - // Hoisted out of the per-result loop by AnalysisScope.TryReplay; see ReplayOnto. - internal List CloneNonHeadsForReplay() - { - return new List(_nonHeadApps.CloneItems()); - } - public Allomorph GetAllomorph(Annotation morph) { var alloID = (string)morph.FeatureStruct.GetValue(HCFeatureSystem.Allomorph); From dae01cb57a701f7fe50fdf9ff15bd08a5e75a4f1 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 25 Sep 2026 18:45:11 -0400 Subject: [PATCH 02/16] test: remove analysis memo coverage Adds focused AnalysisMergeKey regressions covering non-head count and per-rule unapplication counts, the two merge-key dimensions the deleted memo tests exercised. Co-Authored-By: Claude Opus 5.5 --- .../AnalysisMergeKeyTests.cs | 51 ++++ .../AnalysisStateKeyTests.cs | 180 ------------ .../AnalysisStratumRuleTests.cs | 94 ------- .../MemoCorpusVerification.cs | 206 -------------- .../MemoizedCombinationRuleCascadeTests.cs | 197 ------------- .../MergeEquivalentAnalysesTests.cs | 8 +- .../MorpherTests.cs | 260 +----------------- 7 files changed, 57 insertions(+), 939 deletions(-) create mode 100644 tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs delete mode 100644 tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisStateKeyTests.cs delete mode 100644 tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisStratumRuleTests.cs delete mode 100644 tests/SIL.Machine.Morphology.HermitCrab.Tests/MemoCorpusVerification.cs delete mode 100644 tests/SIL.Machine.Morphology.HermitCrab.Tests/MemoizedCombinationRuleCascadeTests.cs diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs new file mode 100644 index 000000000..66d617b5a --- /dev/null +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs @@ -0,0 +1,51 @@ +using NUnit.Framework; +using SIL.Machine.FeatureModel; +using SIL.Machine.Morphology.HermitCrab.MorphologicalRules; + +namespace SIL.Machine.Morphology.HermitCrab; + +// AnalysisMergeKey is internal to AnalysisStratumRule; these tests exercise the merge key +// AnalysisStratumRule.Apply actually uses, not a proxy for it. +[TestFixture] +public class AnalysisMergeKeyTests : HermitCrabTestBase +{ + [Test] + public void Equals_False_WhenNonHeadCountDiffers() + { + Word wordX = NewTestWord(); + wordX.Freeze(); + + Word wordY = NewTestWord(); + Word nonHead = NewTestWord(); + nonHead.Freeze(); + wordY.NonHeadUnapplied(nonHead); + wordY.Freeze(); + + Assert.That(KeysEqual(wordX, wordY), Is.False); + } + + [Test] + public void Equals_False_WhenUnapplicationCountsDiffer() + { + var ruleA = new AffixProcessRule { Name = "ruleA" }; + + Word wordX = NewTestWord(); + wordX.MorphologicalRuleUnapplied(ruleA); + wordX.Freeze(); + + Word wordY = NewTestWord(); + wordY.MorphologicalRuleUnapplied(ruleA); + wordY.MorphologicalRuleUnapplied(ruleA); + wordY.Freeze(); + + Assert.That(KeysEqual(wordX, wordY), Is.False); + } + + private static bool KeysEqual(Word x, Word y) => + new AnalysisStratumRule.AnalysisMergeKey(x).Equals(new AnalysisStratumRule.AnalysisMergeKey(y)); + + private Word NewTestWord() + { + return new Word(Entries["32"].PrimaryAllomorph, FeatureStruct.New().Value) { Stratum = Morphophonemic }; + } +} diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisStateKeyTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisStateKeyTests.cs deleted file mode 100644 index 6c3f39ea7..000000000 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisStateKeyTests.cs +++ /dev/null @@ -1,180 +0,0 @@ -using NUnit.Framework; -using SIL.Machine.FeatureModel; -using SIL.Machine.Morphology.HermitCrab.MorphologicalRules; - -namespace SIL.Machine.Morphology.HermitCrab; - -// The memo's primitives in isolation, independent of any cascade wiring: AnalysisStateKey's -// order-invariance and field sensitivity, its frozen-word requirement, and ReplayOnto's graft. -[TestFixture] -public class AnalysisStateKeyTests : HermitCrabTestBase -{ - [Test] - public void Equals_IsInvariantOverUnapplicationOrder_ForEqualMultisets() - { - var ruleA = new AffixProcessRule { Name = "ruleA" }; - var ruleB = new AffixProcessRule { Name = "ruleB" }; - - // Same multiset {ruleA: 2, ruleB: 1} reached in two different orders. wordY touches ruleB first, - // so the backing dictionaries also differ in insertion order, not just in a repeated rule's - // position. - Word wordX = NewTestWord(); - wordX.MorphologicalRuleUnapplied(ruleA); - wordX.MorphologicalRuleUnapplied(ruleB); - wordX.MorphologicalRuleUnapplied(ruleA); - wordX.Freeze(); - - Word wordY = NewTestWord(); - wordY.MorphologicalRuleUnapplied(ruleB); - wordY.MorphologicalRuleUnapplied(ruleA); - wordY.MorphologicalRuleUnapplied(ruleA); - wordY.Freeze(); - - var keyX = AnalysisStateKey.PinAndKey(wordX); - var keyY = AnalysisStateKey.PinAndKey(wordY); - - Assert.That(keyX.GetHashCode(), Is.EqualTo(keyY.GetHashCode())); - Assert.That(keyX.Equals(keyY), Is.True); - } - - [Test] - public void Equals_False_WhenUnapplicationMultisetsDiffer() - { - var ruleA = new AffixProcessRule { Name = "ruleA" }; - - Word wordX = NewTestWord(); - wordX.MorphologicalRuleUnapplied(ruleA); - wordX.Freeze(); - - Word wordY = NewTestWord(); - wordY.MorphologicalRuleUnapplied(ruleA); - wordY.MorphologicalRuleUnapplied(ruleA); - wordY.Freeze(); - - Assert.That(AnalysisStateKey.PinAndKey(wordX).Equals(AnalysisStateKey.PinAndKey(wordY)), Is.False); - } - - [Test] - public void Equals_False_WhenNonHeadCountDiffers() - { - Word wordX = NewTestWord(); - wordX.Freeze(); - - Word wordY = NewTestWord(); - Word nonHead = NewTestWord(); - nonHead.Freeze(); - wordY.NonHeadUnapplied(nonHead); - wordY.Freeze(); - - Assert.That(AnalysisStateKey.PinAndKey(wordX).Equals(AnalysisStateKey.PinAndKey(wordY)), Is.False); - } - - [Test] - public void Equals_False_WhenSyntacticFeatureStructDiffers() - { - Word wordX = NewTestWord(); - wordX.SyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value; - wordX.Freeze(); - - Word wordY = NewTestWord(); - wordY.SyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("N").Value; - wordY.Freeze(); - - Assert.That(AnalysisStateKey.PinAndKey(wordX).Equals(AnalysisStateKey.PinAndKey(wordY)), Is.False); - } - - [Test] - public void PinAndKey_Throws_WhenWordIsNotFrozen() - { - Word unfrozen = NewTestWord(); - - Assert.That(() => AnalysisStateKey.PinAndKey(unfrozen), Throws.ArgumentException); - } - - [Test] - public void ReplayOnto_SharesHoistedQueryPrefix_AcrossOneHitsReplays() - { - Word queryNonHead = NewTestWord("32"); - queryNonHead.Freeze(); - Word query = NewTestWord("32"); - query.NonHeadUnapplied(queryNonHead); - query.Freeze(); - - Word storedNonHead = NewTestWord("32"); - storedNonHead.Freeze(); - Word subtreeNonHead = NewTestWord("33"); - subtreeNonHead.Freeze(); - Word memoized = NewTestWord("32"); - memoized.NonHeadUnapplied(storedNonHead); - memoized.NonHeadUnapplied(subtreeNonHead); - memoized.Freeze(); - - List hoisted = query.CloneNonHeadsForReplay(); - Word first = memoized.ReplayOnto(query, 0, 1, hoisted); - Word second = memoized.ReplayOnto(query, 0, 1, hoisted); - - // Query's 1 non-head prefix plus the stored subtree's 1 non-head suffix. - Assert.That(first.NonHeadCount, Is.EqualTo(2)); - Assert.That(first.CurrentNonHead.RootAllomorph, Is.SameAs(subtreeNonHead.RootAllomorph)); - // Both replays share the one hoisted clone, and it is a clone rather than the query's own instance. - Assert.That(first.NonHeads[0], Is.SameAs(second.NonHeads[0])); - Assert.That(first.NonHeads[0], Is.SameAs(hoisted[0])); - Assert.That(first.NonHeads[0], Is.Not.SameAs(queryNonHead)); - } - - [Test] - public void ReplayOnto_GraftsQueryPrefixOntoStoredSuffix_ForMruleTrail() - { - var ruleA = new AffixProcessRule { Name = "ruleA" }; - var ruleB = new AffixProcessRule { Name = "ruleB" }; - var ruleC = new AffixProcessRule { Name = "ruleC" }; - - // Trail [ruleA, ruleB] at the moment of the write: ruleA is the length-1 prefix, ruleB the - // subtree-local suffix that must survive the graft. - Word memoized = NewTestWord(); - memoized.MorphologicalRuleUnapplied(ruleA); - memoized.MorphologicalRuleUnapplied(ruleB); - memoized.Freeze(); - - // The same key reached with a different prefix, [ruleC]. - Word query = NewTestWord(); - query.MorphologicalRuleUnapplied(ruleC); - query.Freeze(); - - Word replayed = memoized.ReplayOnto(query, mruleTrailPrefixLength: 1, nonHeadPrefixLength: 0); - - Assert.That(replayed.MorphologicalRules, Is.EqualTo(new IMorphologicalRule[] { ruleC, ruleB })); - } - - [Test] - public void ReplayOnto_GraftsQueryPrefixOntoStoredSuffix_ForNonHeads() - { - // Distinct lexical entries (32 vs 33) so that a graft keeping the wrong non-head, or reversing the - // GetRange window, is distinguishable by RootAllomorph identity rather than only by count. - Word storedNonHead = NewTestWord("32"); - storedNonHead.Freeze(); - Word subtreeNonHead = NewTestWord("33"); - subtreeNonHead.Freeze(); - Word memoized = NewTestWord("32"); - memoized.NonHeadUnapplied(storedNonHead); - memoized.NonHeadUnapplied(subtreeNonHead); - memoized.Freeze(); - - // Query reached the same key with a different (empty) non-head prefix. - Word query = NewTestWord("32"); - query.Freeze(); - - Word replayed = memoized.ReplayOnto(query, mruleTrailPrefixLength: 0, nonHeadPrefixLength: 1); - - // Query's (empty) prefix + the memoized subtree's suffix (subtreeNonHead) = 1 non-head. - Assert.That(replayed.NonHeadCount, Is.EqualTo(1)); - Assert.That(replayed.CurrentNonHead.RootAllomorph, Is.SameAs(subtreeNonHead.RootAllomorph)); - Assert.That(replayed.CurrentNonHead.RootAllomorph, Is.Not.SameAs(storedNonHead.RootAllomorph)); - } - - private Word NewTestWord(string entryId = "32") - { - var word = new Word(Entries[entryId].PrimaryAllomorph, FeatureStruct.New().Value) { Stratum = Morphophonemic }; - return word; - } -} diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisStratumRuleTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisStratumRuleTests.cs deleted file mode 100644 index b9913b2a9..000000000 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisStratumRuleTests.cs +++ /dev/null @@ -1,94 +0,0 @@ -using NUnit.Framework; -using SIL.Machine.Annotations; -using SIL.Machine.FeatureModel; -using SIL.Machine.Matching; -using SIL.Machine.Morphology.HermitCrab.MorphologicalRules; - -namespace SIL.Machine.Morphology.HermitCrab; - -// Drives AnalysisStratumRule against a scope the test owns, which is the only way to observe whether the -// template battery memoized anything: through Morpher the scope is created and discarded inside -// ParseWord, and a Linear stratum invokes the battery at most once per distinct state, so it never -// replays there and the hit counters cannot distinguish a memoized run from an excluded one. -[TestFixture] -public class AnalysisStratumRuleTests : HermitCrabTestBase -{ - [Test] - public void Apply_MemoizesTemplateBattery_OnUnorderedStratum() - { - AddVerbTemplate(); - SetRuleOrder(MorphologicalRuleOrder.Unordered); - - AnalysisScope scope = ApplyStratumRule("sagd"); - - Assert.That( - scope.TemplateMemo, - Is.Not.Empty, - "an Unordered stratum must memoize the template battery -- otherwise the negative case below " - + "proves nothing" - ); - } - - [Test] - public void Apply_DoesNotMemoizeTemplateBattery_OnLinearStratum() - { - AddVerbTemplate(); - SetRuleOrder(MorphologicalRuleOrder.Linear); - - AnalysisScope scope = ApplyStratumRule("sagd"); - - Assert.That( - scope.TemplateMemo, - Is.Empty, - "a Linear stratum must not memoize the template battery: AnalysisStateKey's key-completeness " - + "audit covers only the Unordered cascade" - ); - Assert.That( - scope.Memo, - Is.Empty, - "nor may the mrule table be written on a Linear stratum, which runs PermutationRuleCascade" - ); - } - - // Runs one stratum rule over `word` with a scope attached, and hands the scope back for inspection. - private AnalysisScope ApplyStratumRule(string word) - { - var morpher = new Morpher(TraceManager, Language, maxDegreeOfParallelism: 1); - var stratumRule = new AnalysisStratumRule(morpher, Morphophonemic); - - var input = new Word(Morphophonemic, Morphophonemic.CharacterDefinitionTable.Segment(word)); - var scope = new AnalysisScope(); - input.AnalysisScope = scope; - input.Freeze(); - - // Apply builds its result set eagerly, so this forces the battery for every state it reaches. - _ = stratumRule.Apply(input).ToList(); - return scope; - } - - private void AddVerbTemplate() - { - var any = FeatureStruct.New().Symbol(HCFeatureSystem.Segment).Value; - var dSuffix = new AffixProcessRule - { - Id = "TPAST", - Name = "template_d_suffix", - Gloss = "PAST", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - dSuffix.Allomorphs.Add( - new AffixProcessAllomorph - { - Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, - Rhs = { new CopyFromInput("1"), new InsertSegments(Table3, "+d") }, - } - ); - var verbTemplate = new AffixTemplate - { - Name = "verb_template", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - verbTemplate.Slots.Add(new AffixTemplateSlot(dSuffix) { Optional = true }); - Morphophonemic.AffixTemplates.Add(verbTemplate); - } -} diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MemoCorpusVerification.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MemoCorpusVerification.cs deleted file mode 100644 index e9802516d..000000000 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MemoCorpusVerification.cs +++ /dev/null @@ -1,206 +0,0 @@ -using System.Diagnostics; -using NUnit.Framework; - -namespace SIL.Machine.Morphology.HermitCrab; - -/// -/// Memo-on/memo-off equality against a real grammar, which is the only way to test -/// 's key-completeness audit against the full analysis-side rule set. The -/// synthetic unit-test grammars can force a specific redundant order deterministically but cannot reach -/// that breadth; a key missing a field some real rule reads shows up here and nowhere else. -/// -/// [Explicit] and env-var driven because this repo never commits real grammars or word lists: the test -/// embeds no grammar content and writes only TestContext lines, so no derived corpus data (signature -/// dumps included) can land in a committed path. -/// -/// -/// $env:HC_MEMO_GRAMMAR = "...\sena-hc.xml" -/// $env:HC_MEMO_WORDS = "...\sena-words.txt" -/// $env:HC_MEMO_MAX_WORDS = "60" # optional, default 60 -/// $env:HC_MEMO_TIMEOUT_MS = "5000" # optional, default 5000 (per-word watchdog) -/// dotnet test --filter "FullyQualifiedName~MemoCorpusVerification" -/// -/// -[TestFixture] -[Explicit("Manual corpus verification against a local, uncommitted real grammar; not part of CI.")] -public class MemoCorpusVerification -{ - [Test] - public void MemoOnMatchesMemoOff_AnalysisSetIdentical_OnRealCorpus() - { - (Language language, List words) = Load(); - - var memoOff = new Morpher(new TraceManager(), language); - var memoOn = new Morpher(new TraceManager(), language, maxDegreeOfParallelism: 1); - int timeoutMs = int.TryParse(Environment.GetEnvironmentVariable("HC_MEMO_TIMEOUT_MS"), out int t) ? t : 5000; - - var elapsedMsPerWord = new List(); - var perWordTimes = new List<(string Word, double OnMs, double OffMs)>(); - var divergences = new List(); - var timedOut = new List(); - int noParseBoth = 0; - - foreach (string word in words) - { - List onSignatures; - List offSignatures; - double onMs; - double offMs; - try - { - var swOn = Stopwatch.StartNew(); - onSignatures = RunWithTimeout(() => Signatures(memoOn, word), timeoutMs); - swOn.Stop(); - onMs = swOn.Elapsed.TotalMilliseconds; - - var swOff = Stopwatch.StartNew(); - offSignatures = RunWithTimeout(() => Signatures(memoOff, word), timeoutMs); - swOff.Stop(); - offMs = swOff.Elapsed.TotalMilliseconds; - } - catch (TimeoutException) - { - timedOut.Add(word); - continue; - } - elapsedMsPerWord.Add(onMs + offMs); - perWordTimes.Add((word, onMs, offMs)); - - if (onSignatures.Count == 0 && offSignatures.Count == 0) - noParseBoth++; - - if (!onSignatures.SequenceEqual(offSignatures)) - { - divergences.Add( - $"{word}: memo-on={{{string.Join(",", onSignatures)}}} vs " - + $"memo-off={{{string.Join(",", offSignatures)}}}" - ); - } - } - - elapsedMsPerWord.Sort(); - double p50 = Percentile(elapsedMsPerWord, 0.50); - double p95 = Percentile(elapsedMsPerWord, 0.95); - double totalMs = elapsedMsPerWord.Sum(); - - TestContext.Out.WriteLine($"words attempted: {words.Count}, timed out (>{timeoutMs}ms): {timedOut.Count}"); - TestContext.Out.WriteLine($"words with no parse on both sides: {noParseBoth}"); - TestContext.Out.WriteLine($"aggregate wall: {totalMs:F1} ms, p50: {p50:F1} ms, p95: {p95:F1} ms"); - - // Count-based and wall-clock aggregates stay separate because a corpus is bimodal: many cheap - // words go slightly slower for want of a thread, while a few pathological ones go far faster. One - // combined ratio would hide which regime a reader is in, and both statements are true at once. - int fasterCount = perWordTimes.Count(x => x.OnMs < x.OffMs); - int slowerCount = perWordTimes.Count(x => x.OnMs > x.OffMs); - int tiedCount = perWordTimes.Count - fasterCount - slowerCount; - double totalOnMs = perWordTimes.Sum(x => x.OnMs); - double totalOffMs = perWordTimes.Sum(x => x.OffMs); - TestContext.Out.WriteLine( - $"count-based: {fasterCount}/{perWordTimes.Count} words faster under memo, " - + $"{slowerCount}/{perWordTimes.Count} slower, {tiedCount} tied" - ); - TestContext.Out.WriteLine( - $"wall-clock: memo-on total {totalOnMs:F1} ms vs memo-off total {totalOffMs:F1} ms " - + $"({(totalOnMs > 0 ? totalOffMs / totalOnMs : 0):F2}x)" - ); - if (timedOut.Count > 0) - { - // The ratio above is not a bound in either direction: one try block wraps both calls, so which - // side timed out is unrecorded, and if it was memo-on then that word's memo-off time was never - // measured at all. - TestContext.Out.WriteLine( - $"(the {timedOut.Count} timed-out word(s) above are excluded from both aggregates; " - + "re-run with a higher HC_MEMO_TIMEOUT_MS to actually measure them)" - ); - } - // Per-word attribution as well, since an aggregate dominated by cheap words hides what the - // pathological ones do. Note memo-on is sequential while memo-off is the parallel default, so - // these times measure the user-visible comparison, not the memo's contribution in isolation. - TestContext.Out.WriteLine("heaviest words (by memo-off time), memo-on vs memo-off:"); - foreach ((string w, double onMs2, double offMs2) in perWordTimes.OrderByDescending(x => x.OffMs).Take(10)) - TestContext.Out.WriteLine($" {w}: memo-on {onMs2:F1} ms, memo-off {offMs2:F1} ms"); - TestContext.Out.WriteLine($"mrule memo -- positive hits: {memoOn.MemoHits}, nogood hits: {memoOn.NogoodHits}"); - TestContext.Out.WriteLine( - $"template memo -- positive hits: {memoOn.TemplateMemoHits}, nogood hits: {memoOn.TemplateNogoodHits}" - ); - if (timedOut.Count > 0) - { - // Named rather than counted: these words are excluded from the equality gate, so "0 - // divergences" says nothing about them, and heavy words are exactly what the memo and the - // key-completeness audit most need checking against. - TestContext.Out.WriteLine( - $"timed-out words (excluded from the equality gate above -- re-run with a higher " - + $"HC_MEMO_TIMEOUT_MS to actually check these): {string.Join(", ", timedOut)}" - ); - } - - Assert.That( - divergences, - Is.Empty, - $"{divergences.Count} word(s) diverged between memo-on and memo-off " - + $"(showing up to 10): {string.Join(" | ", divergences.Take(10))}" - ); - Assert.That( - memoOn.MemoHits + memoOn.TemplateMemoHits, - Is.GreaterThan(0), - "the positive replay path must actually have fired somewhere in this corpus -- otherwise " - + "this run cannot distinguish a working memo from a no-op one" - ); - } - - private static List Signatures(Morpher morpher, string word) - { - try - { - return morpher - .ParseWord(word) - .Select(MorpherTests.WordAnalysisSignature) - .OrderBy(s => s, StringComparer.Ordinal) - .ToList(); - } - catch (InvalidShapeException) - { - // As Morpher.AnalyzeWord does: a real word list can contain strings the character table does - // not cover, which both sides reject identically and which tells us nothing about the memo. - return new List(); - } - } - - // Cannot cancel `action`: ParseWord has no cooperative-cancellation hook, so a timed-out word keeps - // running in the background, where it can inflate later words' counters and timings, and enough - // orphaned tasks in a row can starve the thread pool. Tolerable only because this harness never runs - // in CI, and the equality gate excludes timed-out words anyway -- but treat any run that reported - // timeouts as having approximate counts. - private static T RunWithTimeout(Func action, int timeoutMs) - { - Task task = Task.Run(action); - if (!task.Wait(timeoutMs)) - throw new TimeoutException(); - return task.Result; - } - - private static double Percentile(List sortedValues, double fraction) - { - if (sortedValues.Count == 0) - return 0; - int index = (int)Math.Ceiling(fraction * sortedValues.Count) - 1; - return sortedValues[Math.Clamp(index, 0, sortedValues.Count - 1)]; - } - - private static (Language, List) Load() - { - string? grammarPath = Environment.GetEnvironmentVariable("HC_MEMO_GRAMMAR"); - string? wordsPath = Environment.GetEnvironmentVariable("HC_MEMO_WORDS"); - if (string.IsNullOrEmpty(grammarPath) || string.IsNullOrEmpty(wordsPath)) - Assert.Ignore("set HC_MEMO_GRAMMAR and HC_MEMO_WORDS"); - - int maxWords = int.TryParse(Environment.GetEnvironmentVariable("HC_MEMO_MAX_WORDS"), out int mw) ? mw : 60; - Language language = XmlLanguageLoader.Load(grammarPath!); - List words = File.ReadAllLines(wordsPath!) - .Select(w => w.Trim()) - .Where(w => w.Length > 0) - .Take(maxWords) - .ToList(); - return (language, words); - } -} diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MemoizedCombinationRuleCascadeTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MemoizedCombinationRuleCascadeTests.cs deleted file mode 100644 index d2b279998..000000000 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MemoizedCombinationRuleCascadeTests.cs +++ /dev/null @@ -1,197 +0,0 @@ -using NUnit.Framework; -using SIL.Machine.Annotations; -using SIL.Machine.FeatureModel; -using SIL.Machine.Morphology.HermitCrab.MorphologicalRules; -using SIL.Machine.Rules; -using SIL.ObjectModel; - -namespace SIL.Machine.Morphology.HermitCrab; - -// The cascade exercised directly, bypassing Morpher, so a commuting-order re-arrival at a PRODUCTIVE -// state can be forced. That matters because the end-to-end grammars in MorpherTests are small enough -// that they only ever reach the nogood table, leaving the positive replay path untested. -[TestFixture] -public class MemoizedCombinationRuleCascadeTests : HermitCrabTestBase -{ - [Test] - public void Apply_ReplaysPositiveHit_WhenTwoOrdersReachTheSameKey() - { - var ruleA = new AffixProcessRule { Name = "ruleA" }; - var ruleB = new AffixProcessRule { Name = "ruleB" }; - var ruleC = new AffixProcessRule { Name = "ruleC" }; - - // Each rule unapplies at most once, so A-then-B and B-then-A reach the same key (multiset - // {ruleA:1, ruleB:1}) by different routes. ruleC can still apply from that shared state, making - // its subtree positive rather than a nogood. - var cascade = new MemoizedCombinationRuleCascade( - new IRule[] - { - new SingleUseUnapplyRule(ruleA), - new SingleUseUnapplyRule(ruleB), - new SingleUseUnapplyRule(ruleC), - }, - FreezableEqualityComparer.Default - ); - - Word initial = NewTestWord(); - var scope = new AnalysisScope(); - initial.AnalysisScope = scope; - initial.Freeze(); - - List results = new List(cascade.Apply(initial)); - - Assert.That( - results, - Has.Some.Matches(w => - w.GetUnapplicationCount(ruleA) == 1 - && w.GetUnapplicationCount(ruleB) == 1 - && w.GetUnapplicationCount(ruleC) == 1 - ) - ); - Assert.That( - scope.MemoHits, - Is.GreaterThan(0), - "this test's whole point is to force a positive replay -- it must not go vacuous" - ); - } - - [Test] - public void Apply_PositiveReplayMatchesUnmemoizedResultSet_IncludingTrailOrder() - { - // Compares MorphemesInApplicationOrder rather than rule counts: counts are order-invariant, so - // they would pass even if the graft collapsed [ruleB,ruleA,ruleC] into a duplicate of - // [ruleA,ruleB,ruleC], whereas the trail is exactly what ReplayOnto rewrites. - var ruleA = new AffixProcessRule { Id = "RULE_A", Name = "ruleA" }; - var ruleB = new AffixProcessRule { Id = "RULE_B", Name = "ruleB" }; - var ruleC = new AffixProcessRule { Id = "RULE_C", Name = "ruleC" }; - IRule[] rules = - { - new SingleUseUnapplyRule(ruleA), - new SingleUseUnapplyRule(ruleB), - new SingleUseUnapplyRule(ruleC), - }; - - Word memoized = NewTestWord(); - var scope = new AnalysisScope(); - memoized.AnalysisScope = scope; - memoized.Freeze(); - - // No AnalysisScope: takes the unmemoized fallback, the same path a tracing parse takes. - Word unmemoized = NewTestWord(); - unmemoized.Freeze(); - - var memoizedCascade = new MemoizedCombinationRuleCascade(rules, FreezableEqualityComparer.Default); - var unmemoizedCascade = new MemoizedCombinationRuleCascade(rules, FreezableEqualityComparer.Default); - - List memoizedSignatures = memoizedCascade - .Apply(memoized) - .Select(TrailSignature) - .OrderBy(s => s, StringComparer.Ordinal) - .ToList(); - List unmemoizedSignatures = unmemoizedCascade - .Apply(unmemoized) - .Select(TrailSignature) - .OrderBy(s => s, StringComparer.Ordinal) - .ToList(); - - Assert.That( - memoizedSignatures, - Is.EqualTo(unmemoizedSignatures), - "a positive replay must reproduce exactly the unmemoized result set, INCLUDING trail order" - ); - Assert.That( - scope.MemoHits, - Is.GreaterThan(0), - "this test's whole point is to compare a real replay against the unmemoized result -- it " - + "must not go vacuous" - ); - } - - [Test] - public void Apply_FallsBackToUnmemoizedExpansion_WhenKeyIsAlreadyInProgress() - { - // The in-flight state is simulated by pre-populating InProgress, because single-use rules make the - // key monotonic in application count, so a genuine cyclic re-arrival cannot be forced here. - var ruleA = new AffixProcessRule { Id = "RULE_A", Name = "ruleA" }; - var cascade = new MemoizedCombinationRuleCascade( - new IRule[] { new SingleUseUnapplyRule(ruleA) }, - FreezableEqualityComparer.Default - ); - - Word initial = NewTestWord(); - var scope = new AnalysisScope(); - initial.AnalysisScope = scope; - initial.Freeze(); - - var key = AnalysisStateKey.PinAndKey(initial); - scope.InProgress.Add(key); - - List results = new List(cascade.Apply(initial)); - - Assert.That(results, Has.Some.Matches(w => w.GetUnapplicationCount(ruleA) == 1)); - Assert.That( - scope.MemoHits, - Is.Zero, - "the in-flight fallback must not read/count a memo hit -- it never consults Memo at all" - ); - Assert.That( - scope.Memo.ContainsKey(key), - Is.False, - "the in-flight arrival's OWN key must never be written to Memo (deeper recursive calls for " - + "OTHER keys, reached via ApplyRulesRaw's normal recursion, may still memoize themselves)" - ); - } - - [Test] - public void Apply_EnforcesMaxAlternatives_ForRawAndReplayPaths() - { - var ruleA = new AffixProcessRule { Id = "RULE_A", Name = "ruleA" }; - var ruleB = new AffixProcessRule { Id = "RULE_B", Name = "ruleB" }; - IRule[] rules = { new SingleUseUnapplyRule(ruleA), new SingleUseUnapplyRule(ruleB) }; - - var rawCascade = new MemoizedCombinationRuleCascade(rules, FreezableEqualityComparer.Default) - { - MaxAlternatives = 1, - }; - Word rawInitial = NewTestWord(); - rawInitial.AnalysisScope = new AnalysisScope(); - rawInitial.Freeze(); - - Assert.Throws(() => new List(rawCascade.Apply(rawInitial))); - - var replayCascade = new MemoizedCombinationRuleCascade(rules, FreezableEqualityComparer.Default); - Word replayInitial = NewTestWord(); - replayInitial.AnalysisScope = new AnalysisScope(); - replayInitial.Freeze(); - _ = new List(replayCascade.Apply(replayInitial)); - - replayCascade.MaxAlternatives = 1; - Assert.Throws(() => new List(replayCascade.Apply(replayInitial))); - } - - private static string TrailSignature(Word word) => - string.Join("+", word.MorphemesInApplicationOrder.Select(m => m.Id)); - - private Word NewTestWord() - { - return new Word(Entries["32"].PrimaryAllomorph, FeatureStruct.New().Value) { Stratum = Morphophonemic }; - } - - // Stand-in for a compiled analysis rule: unapplies once per input, with no Shape/FeatureStruct - // matching, so commuting orders can be exercised without a real FST-backed rule. - private sealed class SingleUseUnapplyRule(IMorphologicalRule rule) : IRule - { - private readonly IMorphologicalRule _rule = rule; - - public IEnumerable Apply(Word input) - { - if (input.GetUnapplicationCount(_rule) > 0) - yield break; - - Word result = input.Clone(); - result.MorphologicalRuleUnapplied(_rule); - result.Freeze(); - yield return result; - } - } -} diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MergeEquivalentAnalysesTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MergeEquivalentAnalysesTests.cs index dedda6b95..5273d1183 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MergeEquivalentAnalysesTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MergeEquivalentAnalysesTests.cs @@ -6,10 +6,8 @@ namespace SIL.Machine.Morphology.HermitCrab; -// MergeEquivalentAnalyses folds equivalent analyses of a stratum into one canonical word, and only that -// canonical word is unapplied against the strata below. Sharing a shape is therefore not enough to make -// two analyses interchangeable: they have to agree on everything an analysis-side rule reads, which is -// what AnalysisStateKey covers. +// MergeEquivalentAnalyses folds equivalent stratum analyses into one canonical word, then unapplies it +// below. A lower rule may inspect feature state as well as shape. [TestFixture] public class MergeEquivalentAnalysesTests : HermitCrabTestBase { @@ -31,7 +29,7 @@ public void ParseWord_SameShapeDifferentAnalysisState_KeepsBothAnalyses(bool unc AffixProcessRule vRule = Suffix("vRule", v, v, Table1, "t"); AffixProcessRule anyRule = Suffix("anyRule", FeatureStruct.New().Value, v, Table1, "t"); // The cascade runs the stratum's rules in reverse order of registration, and the first analysis to - // reach a given key becomes the canonical word, so registration order decides which one that is. + // reach the same analysis state first becomes canonical, so registration order decides which one. if (unconstrainedFirst) Allophonic.MorphologicalRules.Add(vRule); Allophonic.MorphologicalRules.Add(anyRule); diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs index 1c32d4f1b..94563795f 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs @@ -536,8 +536,7 @@ IList GetNodes(string pattern) return shape.GetNodes(shape.Range).ToList(); } - // A compounding rule and a commuting PAST prefix as peers in one Unordered cascade, so an equal - // AnalysisStateKey can be re-arrived at by different unapplication orders. + // Keeps compound and affix unapplication on the same unordered cascade. private void AddCompoundingAndPrefixRules() { var any = FeatureStruct.New().Symbol(HCFeatureSystem.Segment).Value; @@ -577,7 +576,7 @@ private void AddCompoundingAndPrefixRules() [Test] public void ParseWord_SingleThreaded_MatchesParallel_WithCompounding() { - // MaxDegreeOfParallelism must be a pure no-op on results, independent of the memo it gates. + // Both rule types exercise the sequential and parallel cascade paths. AddCompoundingAndPrefixRules(); var parallel = new Morpher(TraceManager, Language); @@ -595,255 +594,6 @@ public void ParseWord_SingleThreaded_MatchesParallel_WithCompounding() } } - [Test] - public void ParseWord_MemoOnMatchesMemoOff_HitCounterGuarded_WithCompounding() - { - // The standing acceptance gate: analysis-set equality between the memoized sequential cascade and - // the unmemoized parallel default, kept non-vacuous by the hit-counter assertion at the end. - AddCompoundingAndPrefixRules(); - - var memoOff = new Morpher(TraceManager, Language); - var memoOn = new Morpher(TraceManager, Language, maxDegreeOfParallelism: 1); - - foreach (string word in new[] { "pʰutdidat", "pʰutdat" }) - { - List onResult = memoOn.ParseWord(word).ToList(); - List offResult = memoOff.ParseWord(word).ToList(); - Assert.That( - onResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal), - Is.EqualTo(offResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal)), - $"memo-on parse of '{word}' must be analysis-set identical to memo-off" - ); - } - TestContext.Out.WriteLine($"positive hits: {memoOn.MemoHits}, nogood hits: {memoOn.NogoodHits}"); - Assert.That( - memoOn.MemoHits + memoOn.NogoodHits, - Is.GreaterThan(0), - "the memo must actually have hit (positive or nogood) at least once on this grammar -- " - + "otherwise this test cannot distinguish a working memo from a no-op one" - ); - } - - [Test] - public void ParseWord_MemoOnMatchesMemoOff_ForSelfOpaquingSimultaneousEpenthesis() - { - // Guards the memo against a Simultaneous-mode epenthesis rule, which AnalysisRewriteRule compiles - // as ReapplyType.SelfOpaquing -- a repeat-until-fixpoint loop, and the one rule shape whose - // interaction with the nogood cache has a suspected (never reproduced) bug elsewhere. Known gap: - // no available fixture drives the loop past a single iteration, so two or more remains untested. - var highVowel = FeatureStruct - .New(Language.PhonologicalFeatureSystem) - .Symbol(HCFeatureSystem.Segment) - .Symbol("cons-") - .Symbol("voc+") - .Symbol("high+") - .Value; - var highFrontUnrndVowel = FeatureStruct - .New(Language.PhonologicalFeatureSystem) - .Symbol(HCFeatureSystem.Segment) - .Symbol("cons-") - .Symbol("voc+") - .Symbol("high+") - .Symbol("back-") - .Symbol("round-") - .Value; - - var rule4 = new RewriteRule { Name = "rule4", ApplicationMode = RewriteApplicationMode.Simultaneous }; - Allophonic.PhonologicalRules.Add(rule4); - rule4.Subrules.Add( - new RewriteSubrule - { - Rhs = Pattern.New().Annotation(highFrontUnrndVowel).Value, - LeftEnvironment = Pattern.New().Annotation(highVowel).Value, - } - ); - - var memoOff = new Morpher(TraceManager, Language); - var memoOn = new Morpher(TraceManager, Language, maxDegreeOfParallelism: 1); - - foreach (string word in new[] { "buibui", "bubu", "bibu" }) - { - List onResult = memoOn.ParseWord(word).ToList(); - List offResult = memoOff.ParseWord(word).ToList(); - Assert.That( - onResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal), - Is.EqualTo(offResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal)), - $"memo-on parse of '{word}' must be analysis-set identical to memo-off" - ); - } - // Pinned as an absolute value, not just on-vs-off, so a bug affecting both sides identically - // (both wrongly returning empty, say) is still caught. - Assert.That(memoOn.ParseWord("buibui").Count(), Is.EqualTo(1)); - } - - [Test] - public void ParseWord_MemoOnMatchesMemoOff_HitCounterGuarded_WithAffixTemplate() - { - // Two commuting prefixes, not one: a single rule unapplies only once, so no key would ever be - // re-arrived at and the template memo would never fire. Unapplying di-then-gu or gu-then-di - // reaches the same key by a different trail order, which is what makes the second one replay. - var any = FeatureStruct.New().Symbol(HCFeatureSystem.Segment).Value; - - var edSuffix = new AffixProcessRule - { - Id = "TPAST", - Name = "template_ed_suffix", - Gloss = "PAST", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - edSuffix.Allomorphs.Add( - new AffixProcessAllomorph - { - Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, - Rhs = { new CopyFromInput("1"), new InsertSegments(Table3, "+d") }, - } - ); - var verbTemplate = new AffixTemplate - { - Name = "verb_template", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - verbTemplate.Slots.Add(new AffixTemplateSlot(edSuffix) { Optional = true }); - Morphophonemic.AffixTemplates.Add(verbTemplate); - - var diPrefix = new AffixProcessRule - { - Id = "TDI", - Name = "template_di_prefix", - Gloss = "DI", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - diPrefix.Allomorphs.Add( - new AffixProcessAllomorph - { - Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, - Rhs = { new InsertSegments(Table3, "di+"), new CopyFromInput("1") }, - } - ); - Morphophonemic.MorphologicalRules.Add(diPrefix); - - var guPrefix = new AffixProcessRule - { - Id = "TGU", - Name = "template_gu_prefix", - Gloss = "GU", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - guPrefix.Allomorphs.Add( - new AffixProcessAllomorph - { - Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, - Rhs = { new InsertSegments(Table3, "gu+"), new CopyFromInput("1") }, - } - ); - Morphophonemic.MorphologicalRules.Add(guPrefix); - - var memoOff = new Morpher(TraceManager, Language); - var memoOn = new Morpher(TraceManager, Language, maxDegreeOfParallelism: 1); - - foreach (string word in new[] { "digusagd", "disagd", "gusagd", "sagd", "sag" }) - { - List onResult = memoOn.ParseWord(word).ToList(); - List offResult = memoOff.ParseWord(word).ToList(); - Assert.That( - onResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal), - Is.EqualTo(offResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal)), - $"memo-on parse of '{word}' must be analysis-set identical to memo-off" - ); - } - TestContext.Out.WriteLine( - $"template positive hits: {memoOn.TemplateMemoHits}, " - + $"template nogood hits: {memoOn.TemplateNogoodHits}" - ); - // The graft's effect on final signatures is invisible through synthesis, which re-derives rule - // orderings anyway, so this counter -- not the equality assertions above -- is what proves the - // memoized path was exercised at all. - Assert.That( - memoOn.TemplateMemoHits + memoOn.TemplateNogoodHits, - Is.GreaterThan(0), - "the template memo must actually have hit (positive or nogood) at least once on this " - + "grammar -- otherwise this test cannot distinguish a working memo from a no-op one" - ); - } - - [Test] - public void ParseWord_MemoOnMatchesMemoOff_OnLinearStratumWithAffixTemplate() - { - // Every other memo test runs on Unordered strata, leaving Linear -- MorphologicalRuleOrder's - // default -- with no end-to-end equivalence gate. AnalysisStratumRuleTests covers the exclusion - // that keeps the memo off this path; this covers the results it produces. - var any = FeatureStruct.New().Symbol(HCFeatureSystem.Segment).Value; - - var edSuffix = new AffixProcessRule - { - Id = "TPAST", - Name = "template_ed_suffix", - Gloss = "PAST", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - edSuffix.Allomorphs.Add( - new AffixProcessAllomorph - { - Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, - Rhs = { new CopyFromInput("1"), new InsertSegments(Table3, "+d") }, - } - ); - var verbTemplate = new AffixTemplate - { - Name = "verb_template", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - verbTemplate.Slots.Add(new AffixTemplateSlot(edSuffix) { Optional = true }); - Morphophonemic.AffixTemplates.Add(verbTemplate); - - var diPrefix = new AffixProcessRule - { - Id = "TDI", - Name = "template_di_prefix", - Gloss = "DI", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - diPrefix.Allomorphs.Add( - new AffixProcessAllomorph - { - Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, - Rhs = { new InsertSegments(Table3, "di+"), new CopyFromInput("1") }, - } - ); - Morphophonemic.MorphologicalRules.Add(diPrefix); - - var guPrefix = new AffixProcessRule - { - Id = "TGU", - Name = "template_gu_prefix", - Gloss = "GU", - RequiredSyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value, - }; - guPrefix.Allomorphs.Add( - new AffixProcessAllomorph - { - Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, - Rhs = { new InsertSegments(Table3, "gu+"), new CopyFromInput("1") }, - } - ); - Morphophonemic.MorphologicalRules.Add(guPrefix); - - SetRuleOrder(MorphologicalRuleOrder.Linear); - var memoOff = new Morpher(TraceManager, Language); - var memoOn = new Morpher(TraceManager, Language, maxDegreeOfParallelism: 1); - - foreach (string word in new[] { "digusagd", "disagd", "gusagd", "sagd", "sag" }) - { - List onResult = memoOn.ParseWord(word).ToList(); - List offResult = memoOff.ParseWord(word).ToList(); - Assert.That( - onResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal), - Is.EqualTo(offResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal)), - $"Linear-stratum parse of '{word}' must be analysis-set identical with and without the memo" - ); - } - } - [Test] public void ParseWord_HonorsMaxDegreeOfParallelismAsACap_WithoutChangingResults() { @@ -901,11 +651,7 @@ public void CreateParallelOptions_MapsTheCapOntoEveryParallelCallSite() }); } - // What the memo gates compare, instead of object equality: a replayed Word is not field-for-field - // identical to a freshly-computed one. MorphemesInApplicationOrder is the load-bearing part, since it - // walks the trail and non-heads that ReplayOnto rewrites; AllomorphsInMorphOrder alone would miss a - // broken graft, walking only Shape annotations that ReplayOnto never touches. The root distinguishes - // analyses that share a morpheme sequence but not a lexical entry. + // Include allomorph identity, application order, and root identity in result comparisons. internal static string WordAnalysisSignature(Word word) { return string.Join("+", word.AllomorphsInMorphOrder.Select(a => a.Morpheme.Id)) From 3692bb2a8a5ad6e0b78ec6aa11b3b63f27fac0fe Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 25 Sep 2026 18:45:16 -0400 Subject: [PATCH 03/16] docs: remove obsolete memo review guidance Co-Authored-By: Claude Opus 5.5 --- docs/review/devils-advocate.md | 4 ++-- docs/review/hermitcrab.md | 19 +++++++------------ docs/review/machine-tests.md | 6 +++--- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/docs/review/devils-advocate.md b/docs/review/devils-advocate.md index 047d0c5e5..74e7c5d43 100644 --- a/docs/review/devils-advocate.md +++ b/docs/review/devils-advocate.md @@ -17,8 +17,8 @@ Challenge the most consequential claim first, one objection at a time, each with a `path:line` and a concrete scenario. In this repository the claims that have failed before are: -- a HermitCrab performance win asserted without a measured artifact, or a memo - key that omits a field a rule reads; +- a HermitCrab speedup asserted without a measured artifact, or a merge that ignores + state read by an analysis rule; - a USFM or reference change whose test proves the happy path only; - a parity claim about `machine.py` with no checked comparison. diff --git a/docs/review/hermitcrab.md b/docs/review/hermitcrab.md index da050372b..d0a04bf0c 100644 --- a/docs/review/hermitcrab.md +++ b/docs/review/hermitcrab.md @@ -1,20 +1,15 @@ # HermitCrab Review -*Review HermitCrab morphology changes for analysis equivalence, memoization-key -completeness, retained-memory bounds, parallelism, and hot-path cost.* +*Review HermitCrab morphology changes for analysis equivalence, state-sensitive +merging, parallelism, and hot-path cost.* Governs `src/SIL.Machine.Morphology.HermitCrab/**/*.cs`. -- Treat analysis output as the primary contract. A faster parse, more memo hits, or a - successful build does not prove equivalent analyses. -- When changing analysis-side rules or state, re-audit every field the rule reads - against `AnalysisStateKey`. Check freezing, cached hashes, mutable dictionaries, - equality, rule counts, non-head counts, feature structures, and stratum identity. -- Memoized results must represent fully expanded subtrees. Check replay prefixes, - deduplication, empty/nogood entries, in-flight recursion, and the separation between - sequential and parallel scopes. -- Do not weaken an existing memo or retained-word bound without measured evidence - and a test. Read the current limits from the code. +- Treat analysis output as the primary contract. A faster parse or a successful build + does not prove equivalent analyses. +- When changing analysis-side rules or state, check every field a rule reads and + whether equivalent analyses can be merged safely. Check rule counts, non-head + counts, feature structures, and stratum identity. - Inspect allocations and retained object lifetimes only in changed inner loops. If the change claims a performance improvement, require a reproducible benchmark or measured artifact in addition to semantic regression tests. diff --git a/docs/review/machine-tests.md b/docs/review/machine-tests.md index 04d41c4b9..3245ef198 100644 --- a/docs/review/machine-tests.md +++ b/docs/review/machine-tests.md @@ -15,8 +15,8 @@ Governs `tests/**/*.cs`. when the contract is ordinal identity. - For async code, assert cancellation and completion behavior where the change promises it; do not hide unobserved tasks. -- For HermitCrab changes, compare analysis semantics, not only memo-hit counts or - execution success. Exercise key completeness, replay, resource caps, and - parallel/sequential equivalence when touched. +- For HermitCrab changes, compare analysis semantics, not only execution success. + Exercise state-sensitive merging, resource caps, and parallel/sequential + equivalence when touched. - Name the test that proves the change. "Where is the test?" is the single most common review question in this repository; answer it before it is asked. From 2bf5130fdbfae8819c2c3248c800f2d6fd6cdd9a Mon Sep 17 00:00:00 2001 From: John Lambert Date: Tue, 20 Oct 2026 15:19:22 -0400 Subject: [PATCH 04/16] Add FinalTemplateState to AnalysisMergeKey The merge key dropped FinalTemplateState, which AnalysisStateKey carried. Two analyses that differ only in it (unapplying a template rule last leaves None; unapplying a non-template rule last leaves NonTemplate) were merged, and the canonical's NonTemplate then blocked a final template in a lower, non-blocking stratum that the merged-away None alternative would have allowed, losing the parse. Add a parse test that fails without the key change, and a key test. Co-Authored-By: Claude Opus 5.5 --- .../AnalysisStratumRule.cs | 5 ++ .../AffixTemplateTests.cs | 58 +++++++++++++++++++ .../AnalysisMergeKeyTests.cs | 14 +++++ 3 files changed, 77 insertions(+) diff --git a/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs b/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs index 081a686e4..fda22012a 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs @@ -234,6 +234,7 @@ private IEnumerable ApplyTemplates(Word input) private readonly FeatureStruct _realizationalFS; private readonly int _nonHeadCount; private readonly IReadOnlyDictionary _ruleCounts; + private readonly FinalTemplateState _finalTemplateState; private readonly int _hashCode; public AnalysisMergeKey(Word word) @@ -250,6 +251,7 @@ public AnalysisMergeKey(Word word) _realizationalFS = word.RealizationalFeatureStruct; _nonHeadCount = word.NonHeadCount; _ruleCounts = word.UnappliedRuleCounts; + _finalTemplateState = word.FinalTemplateState; _shape.Freeze(); _syntacticFS.Freeze(); @@ -261,6 +263,7 @@ public AnalysisMergeKey(Word word) hash = hash * 31 + _syntacticFS.GetFrozenHashCode(); hash = hash * 31 + _realizationalFS.GetFrozenHashCode(); hash = hash * 31 + _nonHeadCount; + hash = hash * 31 + _finalTemplateState.GetHashCode(); if (_ruleCounts != null) { int multisetHash = 0; @@ -288,6 +291,8 @@ public bool Equals(AnalysisMergeKey other) || !_realizationalFS.ValueEquals(other._realizationalFS) ) return false; + if (_finalTemplateState != other._finalTemplateState) + return false; return RuleCountsEqual(_ruleCounts, other._ruleCounts); } diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AffixTemplateTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AffixTemplateTests.cs index 2c67d81b3..5281410a8 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AffixTemplateTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AffixTemplateTests.cs @@ -733,4 +733,62 @@ public void SameRuleUsedInMultipleTemplates(bool affixHasRequiredFeatures) var morpher = new Morpher(TraceManager, Language); AssertMorphsEqual(morpher.ParseWord("mivd"), "53 IVERB PAST"); } + + [TestCase(0)] + [TestCase(1)] + public void FinalTemplateInLowerStratum_ParsesWhenUpperAnalysesDifferOnlyInFinalTemplateState(int maxParallelism) + { + FeatureStruct any = FeatureStruct.New().Symbol(HCFeatureSystem.Segment).Value; + FeatureStruct verb = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value; + FeatureStruct noun = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("N").Value; + + AffixProcessRule Suffix(string gloss, string segments) + { + var rule = new AffixProcessRule { Name = gloss, Gloss = gloss }; + rule.Allomorphs.Add( + new AffixProcessAllomorph + { + Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, + Rhs = { new CopyFromInput("1"), new InsertSegments(Table3, segments) }, + } + ); + return rule; + } + + // The Morphophonemic stratum does not block final templates, so it inherits the FinalTemplateState the + // Surface stratum leaves on each analysis. + AffixProcessRule finalSuffix = Suffix("FINAL", "d"); + finalSuffix.RequiredSyntacticFeatureStruct = verb; + var finalTemplate = new AffixTemplate { Name = "final", RequiredSyntacticFeatureStruct = verb }; + finalTemplate.Slots.Add(new AffixTemplateSlot(finalSuffix)); + Morphophonemic.AffixTemplates.Add(finalTemplate); + + // Both suffixes add "s", so Surface unapplies them in either order to one shape. NOM last leaves + // NonTemplate, which blocks the final template; TEMPLATE last leaves None, which allows it. + AffixProcessRule templateSuffix = Suffix("TEMPLATE", "s"); + var nonFinalTemplate = new AffixTemplate + { + Name = "non-final", + IsFinal = false, + RequiredSyntacticFeatureStruct = verb, + }; + nonFinalTemplate.Slots.Add(new AffixTemplateSlot(templateSuffix)); + Surface.AffixTemplates.Add(nonFinalTemplate); + + AffixProcessRule nominalizer = Suffix("NOM", "s"); + nominalizer.RequiredSyntacticFeatureStruct = verb; + nominalizer.OutSyntacticFeatureStruct = noun; + Surface.MorphologicalRules.Add(nominalizer); + + var morpher = + maxParallelism == 0 + ? new Morpher(TraceManager, Language) + : new Morpher(TraceManager, Language, maxParallelism); + morpher.AlwaysEnforceFinalTemplates = true; + Assert.That(morpher.MergeEquivalentAnalyses, Is.True); + AssertMorphsEqual(morpher.ParseWord("sagdss"), "32 FINAL TEMPLATE NOM"); + + morpher.MergeEquivalentAnalyses = false; + AssertMorphsEqual(morpher.ParseWord("sagdss"), "32 FINAL TEMPLATE NOM"); + } } diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs index 66d617b5a..b96bbfff5 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs @@ -41,6 +41,20 @@ public void Equals_False_WhenUnapplicationCountsDiffer() Assert.That(KeysEqual(wordX, wordY), Is.False); } + [Test] + public void Equals_False_WhenFinalTemplateStateDiffers() + { + Word wordX = NewTestWord(); + wordX.FinalTemplateState = FinalTemplateState.None; + wordX.Freeze(); + + Word wordY = NewTestWord(); + wordY.FinalTemplateState = FinalTemplateState.NonTemplate; + wordY.Freeze(); + + Assert.That(KeysEqual(wordX, wordY), Is.False); + } + private static bool KeysEqual(Word x, Word y) => new AnalysisStratumRule.AnalysisMergeKey(x).Equals(new AnalysisStratumRule.AnalysisMergeKey(y)); From eaa00abd61e399aa5fbab0fe8b61c722a0648205 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Tue, 20 Oct 2026 15:22:03 -0400 Subject: [PATCH 05/16] Restore single-threaded vs parallel parity tests The sequential CombinationRuleCascade (maxDegreeOfParallelism 1) is still a distinct code path, and only the compounding grammar compared it with the parallel default. Add comparisons for affix templates, a Linear stratum, and Simultaneous epenthesis (buibui parses to one analysis), without the memo hit-counter assertions. Co-Authored-By: Claude Opus 5.5 --- .../MorpherTests.cs | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs index 94563795f..03c29de53 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs @@ -594,6 +594,129 @@ public void ParseWord_SingleThreaded_MatchesParallel_WithCompounding() } } + [Test] + public void ParseWord_SingleThreaded_MatchesParallel_ForSelfOpaquingSimultaneousEpenthesis() + { + // AnalysisRewriteRule compiles a Simultaneous-mode epenthesis rule as ReapplyType.SelfOpaquing, a + // repeat-until-fixpoint loop. No fixture here drives the loop past one iteration. + var highVowel = FeatureStruct + .New(Language.PhonologicalFeatureSystem) + .Symbol(HCFeatureSystem.Segment) + .Symbol("cons-") + .Symbol("voc+") + .Symbol("high+") + .Value; + var highFrontUnrndVowel = FeatureStruct + .New(Language.PhonologicalFeatureSystem) + .Symbol(HCFeatureSystem.Segment) + .Symbol("cons-") + .Symbol("voc+") + .Symbol("high+") + .Symbol("back-") + .Symbol("round-") + .Value; + + var rule4 = new RewriteRule { Name = "rule4", ApplicationMode = RewriteApplicationMode.Simultaneous }; + Allophonic.PhonologicalRules.Add(rule4); + rule4.Subrules.Add( + new RewriteSubrule + { + Rhs = Pattern.New().Annotation(highFrontUnrndVowel).Value, + LeftEnvironment = Pattern.New().Annotation(highVowel).Value, + } + ); + + AssertSingleThreadedMatchesParallel("buibui", "bubu", "bibu"); + // Absolute, so a bug that empties both sides identically is still caught. + Assert.That(new Morpher(TraceManager, Language).ParseWord("buibui").Count(), Is.EqualTo(1)); + } + + [Test] + public void ParseWord_SingleThreaded_MatchesParallel_WithAffixTemplate() + { + AddAffixTemplateAndCommutingPrefixes(); + + AssertSingleThreadedMatchesParallel("digusagd", "disagd", "gusagd", "sagd", "sag"); + Assert.That(new Morpher(TraceManager, Language).ParseWord("digusagd"), Is.Not.Empty); + } + + [Test] + public void ParseWord_SingleThreaded_MatchesParallel_OnLinearStratumWithAffixTemplate() + { + AddAffixTemplateAndCommutingPrefixes(); + SetRuleOrder(MorphologicalRuleOrder.Linear); + + AssertSingleThreadedMatchesParallel("digusagd", "disagd", "gusagd", "sagd", "sag"); + Assert.That(new Morpher(TraceManager, Language).ParseWord("sagd"), Is.Not.Empty); + } + + private void AssertSingleThreadedMatchesParallel(params string[] words) + { + var parallel = new Morpher(TraceManager, Language); + var singleThreaded = new Morpher(TraceManager, Language, maxDegreeOfParallelism: 1); + + foreach (string word in words) + { + List singleResult = singleThreaded.ParseWord(word).ToList(); + List parallelResult = parallel.ParseWord(word).ToList(); + Assert.That( + singleResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal), + Is.EqualTo(parallelResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal)), + $"single-threaded parse of '{word}' must match the parallel parse" + ); + } + } + + // Two commuting prefixes, so one word is reached by more than one unapplication order. + private void AddAffixTemplateAndCommutingPrefixes() + { + var any = FeatureStruct.New().Symbol(HCFeatureSystem.Segment).Value; + var verb = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value; + + var edSuffix = new AffixProcessRule + { + Id = "TPAST", + Name = "template_ed_suffix", + Gloss = "PAST", + RequiredSyntacticFeatureStruct = verb, + }; + edSuffix.Allomorphs.Add( + new AffixProcessAllomorph + { + Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, + Rhs = { new CopyFromInput("1"), new InsertSegments(Table3, "+d") }, + } + ); + var verbTemplate = new AffixTemplate { Name = "verb_template", RequiredSyntacticFeatureStruct = verb }; + verbTemplate.Slots.Add(new AffixTemplateSlot(edSuffix) { Optional = true }); + Morphophonemic.AffixTemplates.Add(verbTemplate); + + foreach ( + (string id, string name, string gloss, string prefix) in new[] + { + ("TDI", "template_di_prefix", "DI", "di+"), + ("TGU", "template_gu_prefix", "GU", "gu+"), + } + ) + { + var prefixRule = new AffixProcessRule + { + Id = id, + Name = name, + Gloss = gloss, + RequiredSyntacticFeatureStruct = verb, + }; + prefixRule.Allomorphs.Add( + new AffixProcessAllomorph + { + Lhs = { Pattern.New("1").Annotation(any).OneOrMore.Value }, + Rhs = { new InsertSegments(Table3, prefix), new CopyFromInput("1") }, + } + ); + Morphophonemic.MorphologicalRules.Add(prefixRule); + } + } + [Test] public void ParseWord_HonorsMaxDegreeOfParallelismAsACap_WithoutChangingResults() { From feeebfed87521f4abc484088801f1d6abb49acba Mon Sep 17 00:00:00 2001 From: John Lambert Date: Tue, 20 Oct 2026 15:24:27 -0400 Subject: [PATCH 06/16] Test AnalysisMergeKey order invariance Two words with the same multiset of unapplied rules reached in different orders must produce equal keys and equal hashes; that is what the XOR multiset combine exists for. Replacing it with a rolling combine now fails this test, and previously passed every AnalysisMergeKey test. Co-Authored-By: Claude Opus 5.5 --- .../AnalysisMergeKeyTests.cs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs index b96bbfff5..9e3929dee 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs @@ -41,6 +41,33 @@ public void Equals_False_WhenUnapplicationCountsDiffer() Assert.That(KeysEqual(wordX, wordY), Is.False); } + [Test] + public void Equals_IsInvariantOverUnapplicationOrder_ForEqualMultisets() + { + var ruleA = new AffixProcessRule { Name = "ruleA" }; + var ruleB = new AffixProcessRule { Name = "ruleB" }; + + // Same multiset {ruleA: 2, ruleB: 1}; wordY touches ruleB first, so the backing dictionaries also + // differ in insertion order, not just in a repeated rule's position. + Word wordX = NewTestWord(); + wordX.MorphologicalRuleUnapplied(ruleA); + wordX.MorphologicalRuleUnapplied(ruleB); + wordX.MorphologicalRuleUnapplied(ruleA); + wordX.Freeze(); + + Word wordY = NewTestWord(); + wordY.MorphologicalRuleUnapplied(ruleB); + wordY.MorphologicalRuleUnapplied(ruleA); + wordY.MorphologicalRuleUnapplied(ruleA); + wordY.Freeze(); + + var keyX = new AnalysisStratumRule.AnalysisMergeKey(wordX); + var keyY = new AnalysisStratumRule.AnalysisMergeKey(wordY); + + Assert.That(keyX.GetHashCode(), Is.EqualTo(keyY.GetHashCode())); + Assert.That(keyX.Equals(keyY), Is.True); + } + [Test] public void Equals_False_WhenFinalTemplateStateDiffers() { From 7c13698eec09d881b5f56697f2e07939529a921f Mon Sep 17 00:00:00 2001 From: John Lambert Date: Tue, 20 Oct 2026 15:25:04 -0400 Subject: [PATCH 07/16] Document that AnalysisMergeKey freezes the syntactic feature struct The constructor mutates its argument by freezing SyntacticFeatureStruct, which Word.FreezeImpl leaves unfrozen; a plain constructor does not signal that, so say so at the constructor. Co-Authored-By: Claude Opus 5.5 --- src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs b/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs index fda22012a..ef48637ac 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs @@ -237,6 +237,7 @@ private IEnumerable ApplyTemplates(Word input) private readonly FinalTemplateState _finalTemplateState; private readonly int _hashCode; + // Freezes word.SyntacticFeatureStruct, which Word.Freeze leaves unfrozen. public AnalysisMergeKey(Word word) { if (!word.IsFrozen) From 72d1a957e2b8a23eddcd1606264a91f8f5b54b8d Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 21 Oct 2026 10:04:23 -0400 Subject: [PATCH 08/16] Make the compounding and Linear-stratum parity tests assert non-empty parses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compounding test compared two empty results for pʰutdidat, and the Linear test compared an empty digusagd; both now assert the words that really parse and say which word does not. Co-Authored-By: Claude Opus 5.5 --- .../MorpherTests.cs | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs index 03c29de53..6026b2e08 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs @@ -579,19 +579,11 @@ public void ParseWord_SingleThreaded_MatchesParallel_WithCompounding() // Both rule types exercise the sequential and parallel cascade paths. AddCompoundingAndPrefixRules(); - var parallel = new Morpher(TraceManager, Language); - var singleThreaded = new Morpher(TraceManager, Language, maxDegreeOfParallelism: 1); - - foreach (string word in new[] { "pʰutdidat", "pʰutdat" }) - { - List singleResult = singleThreaded.ParseWord(word).ToList(); - List parallelResult = parallel.ParseWord(word).ToList(); - Assert.That( - singleResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal), - Is.EqualTo(parallelResult.Select(WordAnalysisSignature).OrderBy(s => s, StringComparer.Ordinal)), - $"single-threaded parse of '{word}' must match the parallel parse" - ); - } + string[] words = { "pʰutdidat", "pʰutdat" }; + AssertSingleThreadedMatchesParallel(words); + // Absolute, so a bug that empties both sides identically is still caught. pʰutdidat has no parse here, + // so only pʰutdat (two compound analyses) is compared non-trivially. + Assert.That(new Morpher(TraceManager, Language).ParseWord("pʰutdat"), Is.Not.Empty); } [Test] @@ -647,7 +639,12 @@ public void ParseWord_SingleThreaded_MatchesParallel_OnLinearStratumWithAffixTem SetRuleOrder(MorphologicalRuleOrder.Linear); AssertSingleThreadedMatchesParallel("digusagd", "disagd", "gusagd", "sagd", "sag"); - Assert.That(new Morpher(TraceManager, Language).ParseWord("sagd"), Is.Not.Empty); + // With the Linear order the di and gu prefixes cannot both apply, so digusagd has no parse. The other + // four words parse, and the parity check on digusagd is only an empty-equals-empty comparison. + var parallel = new Morpher(TraceManager, Language); + Assert.That(parallel.ParseWord("digusagd"), Is.Empty); + foreach (string word in new[] { "disagd", "gusagd", "sagd", "sag" }) + Assert.That(parallel.ParseWord(word), Is.Not.Empty, $"'{word}' must parse"); } private void AssertSingleThreadedMatchesParallel(params string[] words) @@ -667,7 +664,8 @@ private void AssertSingleThreadedMatchesParallel(params string[] words) } } - // Two commuting prefixes, so one word is reached by more than one unapplication order. + // Two prefixes that commute on an Unordered stratum, so one word is reached by more than one unapplication + // order. On a Linear stratum they cannot both apply, so the double-prefixed word has no parse. private void AddAffixTemplateAndCommutingPrefixes() { var any = FeatureStruct.New().Symbol(HCFeatureSystem.Segment).Value; From 0b10e4ec4c5c5f653f9537738b25aff68aae1658 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 21 Oct 2026 10:04:41 -0400 Subject: [PATCH 09/16] Make the review checklist and MaxDegreeOfParallelism doc match the code The hermitcrab checklist lists exactly the AnalysisMergeKey fields, MaxDegreeOfParallelism is documented again as a results-neutral performance knob, and the merge-order test comment is stated in terms of AnalysisMergeKey. Co-Authored-By: Claude Opus 5.5 --- docs/review/hermitcrab.md | 6 ++++-- src/SIL.Machine.Morphology.HermitCrab/Morpher.cs | 4 +++- .../MergeEquivalentAnalysesTests.cs | 4 ++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/docs/review/hermitcrab.md b/docs/review/hermitcrab.md index d0a04bf0c..6bc033c7e 100644 --- a/docs/review/hermitcrab.md +++ b/docs/review/hermitcrab.md @@ -8,8 +8,10 @@ Governs `src/SIL.Machine.Morphology.HermitCrab/**/*.cs`. - Treat analysis output as the primary contract. A faster parse or a successful build does not prove equivalent analyses. - When changing analysis-side rules or state, check every field a rule reads and - whether equivalent analyses can be merged safely. Check rule counts, non-head - counts, feature structures, and stratum identity. + whether equivalent analyses can be merged safely. The merge key + (`AnalysisMergeKey`) compares exactly these fields: shape, stratum identity, syntactic + feature structure, realizational feature structure, non-head count, per-rule + unapplication counts, and `FinalTemplateState`. Check a new field against that list. - Inspect allocations and retained object lifetimes only in changed inner loops. If the change claims a performance improvement, require a reproducible benchmark or measured artifact in addition to semantic regression tests. diff --git a/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs b/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs index 864581961..4f70cfa21 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs @@ -117,7 +117,9 @@ public ITraceManager TraceManager /// /// Caps concurrency within a parse or generation, including the analysis cascade, affix-template /// unapplication, and synthesis. A value of 1 runs the work sequentially; values below 1 leave - /// concurrency unbounded. This is constructor-only because it determines how the analysis rules compile. + /// concurrency unbounded. This is constructor-only because it determines how the analysis rules compile + /// (a value of 1 compiles the sequential cascade for unordered strata). It is a performance knob only: + /// every value produces equal parse results, though the work may run through a different path. /// public int MaxDegreeOfParallelism { get; } diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MergeEquivalentAnalysesTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MergeEquivalentAnalysesTests.cs index 5273d1183..98831d09c 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MergeEquivalentAnalysesTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MergeEquivalentAnalysesTests.cs @@ -28,8 +28,8 @@ public void ParseWord_SameShapeDifferentAnalysisState_KeepsBothAnalyses(bool unc AffixProcessRule vRule = Suffix("vRule", v, v, Table1, "t"); AffixProcessRule anyRule = Suffix("anyRule", FeatureStruct.New().Value, v, Table1, "t"); - // The cascade runs the stratum's rules in reverse order of registration, and the first analysis to - // reach the same analysis state first becomes canonical, so registration order decides which one. + // The cascade runs the stratum's rules in reverse order of registration, so registration order sets + // which same-shape analysis reaches the merge first. Their AnalysisMergeKeys differ, so both stay. if (unconstrainedFirst) Allophonic.MorphologicalRules.Add(vRule); Allophonic.MorphologicalRules.Add(anyRule); From 677da87e63dea07423e425d10cc335d0f1ec1c3c Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 21 Oct 2026 10:12:49 -0400 Subject: [PATCH 10/16] =?UTF-8?q?Assert=20p=CA=B0utdat=20parses=20in=20the?= =?UTF-8?q?=20parallelism-cap=20test=20so=20it=20cannot=20pass=20vacuously?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parallelism-cap test compared capped and unbounded results only, so two empty results for pʰutdat would pass. It now asserts the unbounded parse of pʰutdat is non-empty, matching the compounding parity test. Co-Authored-By: Claude Opus 5.5 --- tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs index 6026b2e08..77f35d9fc 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs @@ -734,6 +734,10 @@ public void ParseWord_HonorsMaxDegreeOfParallelismAsACap_WithoutChangingResults( $"a parallelism cap must not change the analysis set for '{word}'" ); } + + // Absolute, so a bug that empties both capped and unbounded results identically is still caught. + // pʰutdidat has no parse here, so only pʰutdat (two analyses) is compared non-trivially. + Assert.That(unbounded.ParseWord("pʰutdat"), Is.Not.Empty); } [Test] From d071f3cca845be8af8cca005286de5bf550bb45a Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 21 Oct 2026 15:45:29 -0400 Subject: [PATCH 11/16] Caveat MaxAlternatives in the MaxDegreeOfParallelism doc The doc said every value produces equal parse results. With MaxAlternatives set the parallel cascade counts alternatives before removing duplicates, so it can throw where a value of 1 parses. The doc is a rule again, as on master, and names that exception. Co-Authored-By: Claude Opus 5.5 --- src/SIL.Machine.Morphology.HermitCrab/Morpher.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs b/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs index 4f70cfa21..11c4e129b 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs @@ -118,8 +118,11 @@ public ITraceManager TraceManager /// Caps concurrency within a parse or generation, including the analysis cascade, affix-template /// unapplication, and synthesis. A value of 1 runs the work sequentially; values below 1 leave /// concurrency unbounded. This is constructor-only because it determines how the analysis rules compile - /// (a value of 1 compiles the sequential cascade for unordered strata). It is a performance knob only: - /// every value produces equal parse results, though the work may run through a different path. + /// (a value of 1 compiles the sequential cascade for unordered strata). It must remain a performance + /// knob only: nothing that changes which analyses a parse returns may depend on it. One known exception + /// predates this rule: with set, the parallel cascade counts alternatives + /// before removing duplicates, so it can throw on a word + /// that a value of 1 parses. /// public int MaxDegreeOfParallelism { get; } From f34341e6cdfab2b4490b3275c387ab5507300696 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 21 Oct 2026 16:09:53 -0400 Subject: [PATCH 12/16] Port the syntactic-feature and unfrozen-word key tests to AnalysisMergeKey AnalysisStateKeyTests covered both behaviors; the port dropped them, so removing either from AnalysisMergeKey passed every test. Co-Authored-By: Claude Opus 5.5 --- .../AnalysisMergeKeyTests.cs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs index 9e3929dee..25eac5ef1 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/AnalysisMergeKeyTests.cs @@ -82,6 +82,28 @@ public void Equals_False_WhenFinalTemplateStateDiffers() Assert.That(KeysEqual(wordX, wordY), Is.False); } + [Test] + public void Equals_False_WhenSyntacticFeatureStructDiffers() + { + Word wordX = NewTestWord(); + wordX.SyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("V").Value; + wordX.Freeze(); + + Word wordY = NewTestWord(); + wordY.SyntacticFeatureStruct = FeatureStruct.New(Language.SyntacticFeatureSystem).Symbol("N").Value; + wordY.Freeze(); + + Assert.That(KeysEqual(wordX, wordY), Is.False); + } + + [Test] + public void Constructor_Throws_WhenWordIsNotFrozen() + { + Word unfrozen = NewTestWord(); + + Assert.That(() => new AnalysisStratumRule.AnalysisMergeKey(unfrozen), Throws.ArgumentException); + } + private static bool KeysEqual(Word x, Word y) => new AnalysisStratumRule.AnalysisMergeKey(x).Equals(new AnalysisStratumRule.AnalysisMergeKey(y)); From 2cbf499c4c30056d24ce04ec18e1d7b3676f89b5 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Wed, 21 Oct 2026 20:12:54 -0400 Subject: [PATCH 13/16] Pin gudisagd in the Linear-stratum parity test On a Linear stratum the di and gu prefixes still both apply, but only in rule order, so gudisagd parses and digusagd does not. The test and the fixture comment said neither nesting parsed; the test now asserts gudisagd is non-empty and compares it across both cascades. Co-Authored-By: Claude Opus 5.5 --- .../MorpherTests.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs index 77f35d9fc..d9149d058 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorpherTests.cs @@ -638,12 +638,12 @@ public void ParseWord_SingleThreaded_MatchesParallel_OnLinearStratumWithAffixTem AddAffixTemplateAndCommutingPrefixes(); SetRuleOrder(MorphologicalRuleOrder.Linear); - AssertSingleThreadedMatchesParallel("digusagd", "disagd", "gusagd", "sagd", "sag"); - // With the Linear order the di and gu prefixes cannot both apply, so digusagd has no parse. The other - // four words parse, and the parity check on digusagd is only an empty-equals-empty comparison. + AssertSingleThreadedMatchesParallel("digusagd", "gudisagd", "disagd", "gusagd", "sagd", "sag"); + // With the Linear order the di and gu prefixes apply only in rule order, so gudisagd parses and + // digusagd has no parse. The parity check on digusagd is only an empty-equals-empty comparison. var parallel = new Morpher(TraceManager, Language); Assert.That(parallel.ParseWord("digusagd"), Is.Empty); - foreach (string word in new[] { "disagd", "gusagd", "sagd", "sag" }) + foreach (string word in new[] { "gudisagd", "disagd", "gusagd", "sagd", "sag" }) Assert.That(parallel.ParseWord(word), Is.Not.Empty, $"'{word}' must parse"); } @@ -665,7 +665,7 @@ private void AssertSingleThreadedMatchesParallel(params string[] words) } // Two prefixes that commute on an Unordered stratum, so one word is reached by more than one unapplication - // order. On a Linear stratum they cannot both apply, so the double-prefixed word has no parse. + // order. On a Linear stratum they apply only in rule order, so only gudisagd parses. private void AddAffixTemplateAndCommutingPrefixes() { var any = FeatureStruct.New().Symbol(HCFeatureSystem.Segment).Value; From 4f3ea6ab0a1fe995e89c9e121a28a35af2d4df7a Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 22 Oct 2026 06:57:56 -0400 Subject: [PATCH 14/16] Count distinct results toward MaxAlternatives in parallel cascades ParallelCombinationRuleCascade and ParallelPermutationRuleCascade added every rule output to the count before removing duplicates, so with MaxAlternatives set they could throw where the sequential cascades, which count a set, do not. Collect output in a set and count only new results. The MaxDegreeOfParallelism doc no longer needs the exception. Co-Authored-By: Claude Opus 5.5 --- .../Morpher.cs | 6 +- .../Rules/ParallelCombinationRuleCascade.cs | 12 ++- .../Rules/ParallelPermutationRuleCascade.cs | 12 ++- .../Rules/RuleCascadeTests.cs | 74 +++++++++++++++++++ 4 files changed, 92 insertions(+), 12 deletions(-) create mode 100644 tests/SIL.Machine.Tests/Rules/RuleCascadeTests.cs diff --git a/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs b/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs index 11c4e129b..83023517e 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/Morpher.cs @@ -119,10 +119,8 @@ public ITraceManager TraceManager /// unapplication, and synthesis. A value of 1 runs the work sequentially; values below 1 leave /// concurrency unbounded. This is constructor-only because it determines how the analysis rules compile /// (a value of 1 compiles the sequential cascade for unordered strata). It must remain a performance - /// knob only: nothing that changes which analyses a parse returns may depend on it. One known exception - /// predates this rule: with set, the parallel cascade counts alternatives - /// before removing duplicates, so it can throw on a word - /// that a value of 1 parses. + /// knob only: nothing that changes which analyses a parse returns, or whether + /// is exceeded, may depend on it. /// public int MaxDegreeOfParallelism { get; } diff --git a/src/SIL.Machine/Rules/ParallelCombinationRuleCascade.cs b/src/SIL.Machine/Rules/ParallelCombinationRuleCascade.cs index b126b8725..de869978c 100644 --- a/src/SIL.Machine/Rules/ParallelCombinationRuleCascade.cs +++ b/src/SIL.Machine/Rules/ParallelCombinationRuleCascade.cs @@ -42,7 +42,8 @@ public override IEnumerable Apply(TData input) { MaxDegreeOfParallelism = MaxDegreeOfParallelism >= 1 ? MaxDegreeOfParallelism : -1, }; - var output = new ConcurrentStack(); + // A set, so MaxAlternatives counts distinct results as the sequential cascade does. + var output = new ConcurrentDictionary(Comparer); var from = new ConcurrentStack>>(); from.Push(Tuple.Create(input, !MultipleApplication ? new HashSet() : null)); var to = new ConcurrentStack>>(); @@ -65,8 +66,11 @@ public override IEnumerable Apply(TData input) TData[] results = ApplyRule(Rules[i], i, work.Item1).ToArray(); if (results.Length > 0) { - output.PushRange(results); - CheckMaxAlternatives(Interlocked.Add(ref alternativeCount, results.Length)); + foreach (TData result in results) + { + if (output.TryAdd(result, 0)) + CheckMaxAlternatives(Interlocked.Increment(ref alternativeCount)); + } Tuple>[] workItems = results .Where(res => !Comparer.Equals(work.Item1, res)) @@ -97,7 +101,7 @@ public override IEnumerable Apply(TData input) to = temp; } - return output.Distinct(Comparer); + return output.Keys; } } } diff --git a/src/SIL.Machine/Rules/ParallelPermutationRuleCascade.cs b/src/SIL.Machine/Rules/ParallelPermutationRuleCascade.cs index d12a58dfa..8d026c3a6 100644 --- a/src/SIL.Machine/Rules/ParallelPermutationRuleCascade.cs +++ b/src/SIL.Machine/Rules/ParallelPermutationRuleCascade.cs @@ -32,7 +32,8 @@ IEqualityComparer comparer public override IEnumerable Apply(TData input) { - var output = new ConcurrentStack(); + // A set, so MaxAlternatives counts distinct results as the sequential cascade does. + var output = new ConcurrentDictionary(Comparer); var from = new ConcurrentStack>(); from.Push(Tuple.Create(input, 0)); var to = new ConcurrentStack>(); @@ -52,8 +53,11 @@ public override IEnumerable Apply(TData input) TData[] results = ApplyRule(Rules[i], i, work.Item1).ToArray(); if (results.Length > 0) { - output.PushRange(results); - CheckMaxAlternatives(Interlocked.Add(ref alternativeCount, results.Length)); + foreach (TData result in results) + { + if (output.TryAdd(result, 0)) + CheckMaxAlternatives(Interlocked.Increment(ref alternativeCount)); + } Tuple[] workItems = results .Where(res => !MultipleApplication || !Comparer.Equals(work.Item1, res)) @@ -78,7 +82,7 @@ public override IEnumerable Apply(TData input) to = temp; } - return output.Distinct(Comparer); + return output.Keys; } } } diff --git a/tests/SIL.Machine.Tests/Rules/RuleCascadeTests.cs b/tests/SIL.Machine.Tests/Rules/RuleCascadeTests.cs new file mode 100644 index 000000000..1c2c1924f --- /dev/null +++ b/tests/SIL.Machine.Tests/Rules/RuleCascadeTests.cs @@ -0,0 +1,74 @@ +using NUnit.Framework; +using SIL.Machine.Annotations; + +namespace SIL.Machine.Rules; + +[TestFixture] +public class RuleCascadeTests +{ + private static readonly IEqualityComparer Comparer = new StringDataComparer(); + + // Both rules rewrite any other string to "b", so the cascade sees "b" twice but has one distinct result. + private static IRule[] DuplicateProducingRules() => [new RewriteToB(), new RewriteToB()]; + + [Test] + public void ParallelCombination_MaxAlternatives_CountsDistinctResults() + { + var sequential = new CombinationRuleCascade(DuplicateProducingRules(), Comparer) + { + MaxAlternatives = 1, + }; + var parallel = new ParallelCombinationRuleCascade(DuplicateProducingRules(), Comparer) + { + MaxAlternatives = 1, + }; + + AssertSameResults(sequential, parallel); + } + + [Test] + public void ParallelPermutation_MaxAlternatives_CountsDistinctResults() + { + var sequential = new PermutationRuleCascade(DuplicateProducingRules(), Comparer) + { + MaxAlternatives = 1, + }; + var parallel = new ParallelPermutationRuleCascade(DuplicateProducingRules(), Comparer) + { + MaxAlternatives = 1, + }; + + AssertSameResults(sequential, parallel); + } + + private static void AssertSameResults( + RuleCascade sequential, + RuleCascade parallel + ) + { + string[] expected = sequential.Apply(new AnnotatedStringData("a")).Select(d => d.String).ToArray(); + Assert.That(expected, Is.EqualTo(new[] { "b" })); + Assert.That( + parallel.Apply(new AnnotatedStringData("a")).Select(d => d.String).ToArray(), + Is.EquivalentTo(expected) + ); + } + + private sealed class RewriteToB : IRule + { + public IEnumerable Apply(AnnotatedStringData input) + { + if (string.Equals(input.String, "b", StringComparison.Ordinal)) + return []; + return [new AnnotatedStringData("b")]; + } + } + + private sealed class StringDataComparer : IEqualityComparer + { + public bool Equals(AnnotatedStringData? x, AnnotatedStringData? y) => + string.Equals(x?.String, y?.String, StringComparison.Ordinal); + + public int GetHashCode(AnnotatedStringData obj) => StringComparer.Ordinal.GetHashCode(obj.String); + } +} From 9740b96509fc0da36654e3ab5d7d2eca1cad776c Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 22 Oct 2026 15:48:22 -0400 Subject: [PATCH 15/16] Drop AnalysisScope from the copy-pruning tests #519 landed on master with tests that set Word.AnalysisScope, which this branch removes along with the memo. The transform does not read it. Co-Authored-By: Claude Opus 5.5 --- .../PruneDisagreeingCopiesTests.cs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorphologicalRules/PruneDisagreeingCopiesTests.cs b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorphologicalRules/PruneDisagreeingCopiesTests.cs index d532e6457..ffd6fd9f8 100644 --- a/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorphologicalRules/PruneDisagreeingCopiesTests.cs +++ b/tests/SIL.Machine.Morphology.HermitCrab.Tests/MorphologicalRules/PruneDisagreeingCopiesTests.cs @@ -51,7 +51,7 @@ public void TracingStillPrunesDisagreeingCopies() { (string first, string second) = FindIncompatibleSegments(); AffixProcessRule rule = CreateCopyRule(SingleSegmentPart()); - var input = new Word(Surface, Table3.Segment(first + second)) { AnalysisScope = new AnalysisScope() }; + var input = new Word(Surface, Table3.Segment(first + second)); TraceManager.AnalyzeWord(Language, input); input.Freeze(); TraceManager.IsTracing = true; @@ -74,7 +74,7 @@ public void EveryPairOfCopiesIsCompared(bool pruneCopies, int expectedOutputs) // the first copy is underspecified, as unapplied phonology can leave it, so it unifies with both others (string first, string second) = FindIncompatibleSegments(); AffixProcessRule rule = CreateCopyRule(SingleSegmentPart(), copyCount: 3); - var input = new Word(Surface, Table3.Segment(first + first + second)) { AnalysisScope = new AnalysisScope() }; + var input = new Word(Surface, Table3.Segment(first + first + second)); input.Shape.First.Annotation.FeatureStruct = AnySegment.Clone(); input.Freeze(); @@ -127,7 +127,7 @@ public void OptionalNonSegmentInACopyIsNeverPruned() { (string first, string second) = FindIncompatibleSegments(); AffixProcessRule rule = CreateCopyRule(FullPart()); - var input = new Word(Surface, Table3.Segment(first + second + second)) { AnalysisScope = new AnalysisScope() }; + var input = new Word(Surface, Table3.Segment(first + second + second)); input.Shape.AddAfter(input.Shape.First.Next, FeatureStruct.New().Symbol(HCFeatureSystem.Boundary).Value, true); input.Freeze(); @@ -143,10 +143,7 @@ public void WordInitialOptionalNonSegmentIsNeverPruned() // the matcher skips the leading boundary, so it sits before the first copy rather than inside it (string first, string second) = FindIncompatibleSegments(); AffixProcessRule rule = CreateCopyRule(FullPart()); - var input = new Word(Surface, Table3.Segment(first + second + first + second)) - { - AnalysisScope = new AnalysisScope(), - }; + var input = new Word(Surface, Table3.Segment(first + second + first + second)); input.Shape.AddAfter(input.Shape.Begin, FeatureStruct.New().Symbol(HCFeatureSystem.Boundary).Value, true); input.Freeze(); @@ -618,7 +615,7 @@ private string[] AnalysisForms(bool pruneCopies, AffixProcessRule rule, Word inp private Word CreateInputWithOptionalSecondSegment(string representation) { - var input = new Word(Surface, Table3.Segment(representation)) { AnalysisScope = new AnalysisScope() }; + var input = new Word(Surface, Table3.Segment(representation)); input.Shape.First.Next.Annotation.Optional = true; input.Freeze(); return input; @@ -626,7 +623,7 @@ private Word CreateInputWithOptionalSecondSegment(string representation) private Word CreateInputWithOptionalFirstSegment(string representation) { - var input = new Word(Surface, Table3.Segment(representation)) { AnalysisScope = new AnalysisScope() }; + var input = new Word(Surface, Table3.Segment(representation)); input.Shape.First.Annotation.Optional = true; input.Freeze(); return input; @@ -634,7 +631,7 @@ private Word CreateInputWithOptionalFirstSegment(string representation) private Word CreateInput(string representation) { - var input = new Word(Surface, Table3.Segment(representation)) { AnalysisScope = new AnalysisScope() }; + var input = new Word(Surface, Table3.Segment(representation)); input.Freeze(); return input; } From a00aaea53ed212f23f49e05b9a534c2a081c66f7 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Thu, 22 Oct 2026 15:48:23 -0400 Subject: [PATCH 16/16] Default a null comparer in RuleCascade The parallel cascades now build a ConcurrentDictionary from Comparer, whose netstandard2.0 constructor rejects null; before, Distinct(null) fell back to the default. ParallelCombinationRuleCascade also called Comparer.Equals directly, so a null comparer already crashed there. Co-Authored-By: Claude Opus 5.5 --- src/SIL.Machine/Rules/RuleCascade.cs | 2 +- .../SIL.Machine.Tests/Rules/RuleCascadeTests.cs | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/SIL.Machine/Rules/RuleCascade.cs b/src/SIL.Machine/Rules/RuleCascade.cs index 179588b22..80cc40800 100644 --- a/src/SIL.Machine/Rules/RuleCascade.cs +++ b/src/SIL.Machine/Rules/RuleCascade.cs @@ -30,7 +30,7 @@ IEqualityComparer comparer { _rules = new ReadOnlyList>(rules.ToList()); _multiApp = multiApp; - _comparer = comparer; + _comparer = comparer ?? EqualityComparer.Default; } public IEqualityComparer Comparer diff --git a/tests/SIL.Machine.Tests/Rules/RuleCascadeTests.cs b/tests/SIL.Machine.Tests/Rules/RuleCascadeTests.cs index 1c2c1924f..3185a2cdb 100644 --- a/tests/SIL.Machine.Tests/Rules/RuleCascadeTests.cs +++ b/tests/SIL.Machine.Tests/Rules/RuleCascadeTests.cs @@ -41,6 +41,22 @@ public void ParallelPermutation_MaxAlternatives_CountsDistinctResults() AssertSameResults(sequential, parallel); } + [Test] + public void Constructor_NullComparer_UsesDefaultComparer() + { + var combination = new ParallelCombinationRuleCascade( + DuplicateProducingRules(), + null! + ); + var permutation = new ParallelPermutationRuleCascade( + DuplicateProducingRules(), + null! + ); + + Assert.That(combination.Comparer, Is.SameAs(EqualityComparer.Default)); + Assert.That(permutation.Comparer, Is.SameAs(EqualityComparer.Default)); + } + private static void AssertSameResults( RuleCascade sequential, RuleCascade parallel