From d3547643a94f16d37a3787e655c9ff32adb1158d Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 25 Sep 2026 18:45:02 -0400 Subject: [PATCH 1/3] refactor: remove analysis cascade memo Co-Authored-By: Claude Opus 5.5 --- .../AnalysisScope.cs | 134 --------------- .../AnalysisStateKey.cs | 126 -------------- .../AnalysisStratumRule.cs | 156 ++++++++++++------ .../MemoizedCombinationRuleCascade.cs | 118 ------------- .../Morpher.cs | 47 +----- src/SIL.Machine.Morphology.HermitCrab/Word.cs | 98 +---------- 6 files changed, 115 insertions(+), 564 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 e5461095f..000000000 --- a/src/SIL.Machine.Morphology.HermitCrab/AnalysisStateKey.cs +++ /dev/null @@ -1,126 +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. - /// : adds - /// (MaxStemCount gate) -- never the non-heads' own content, only the count. - /// : 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 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; - - // 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; - 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; - 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 6fb98916c..c92227856 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; } } @@ -127,11 +135,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)); @@ -150,10 +158,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); @@ -190,38 +198,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) { @@ -243,5 +222,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 f55be0b49..6cd55e062 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) { @@ -83,21 +79,15 @@ 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; } /// - /// 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; } @@ -116,24 +106,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; } @@ -165,10 +137,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); @@ -176,8 +144,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(); @@ -435,9 +401,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); @@ -510,8 +473,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 455ba4eff..6755e8e45 100644 --- a/src/SIL.Machine.Morphology.HermitCrab/Word.cs +++ b/src/SIL.Machine.Morphology.HermitCrab/Word.cs @@ -70,11 +70,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; @@ -89,13 +84,12 @@ 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; _isPartial = word._isPartial; CurrentTrace = word.CurrentTrace; - AnalysisScope = word.AnalysisScope; _disjunctiveAllomorphIndices = word._disjunctiveAllomorphIndices.ToDictionary( kvp => kvp.Key, kvp => new HashSet(kvp.Value) @@ -218,16 +212,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; } @@ -329,12 +313,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) { @@ -360,9 +338,6 @@ internal int GetUnapplicationCount(IMorphologicalRule mrule) return numUnapplies; } - /// - /// The full multiset backing , for . - /// internal IReadOnlyDictionary UnappliedRuleCounts => _mrulesUnapplied; /// @@ -419,15 +394,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(); @@ -486,68 +452,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 650223db1167cd326ed531fff513d4c1030e1f89 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 25 Sep 2026 18:45:11 -0400 Subject: [PATCH 2/3] 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 a84dc6fd4ec3e2e8dde1713af4bb8b8d03b26187 Mon Sep 17 00:00:00 2001 From: John Lambert Date: Fri, 25 Sep 2026 18:45:16 -0400 Subject: [PATCH 3/3] 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.