Repository navigation
Remove HermitCrab's analysis-cascade memo - #522
johnml1135 wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #522 +/- ##
==========================================
- Coverage 74.36% 74.19% -0.17%
==========================================
Files 456 453 -3
Lines 38325 38157 -168
Branches 5263 5240 -23
==========================================
- Hits 28499 28312 -187
- Misses 8669 8684 +15
- Partials 1157 1161 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4490f59 to
a84dc6f
Compare
a84dc6f to
8313ba0
Compare
| private readonly IReadOnlyDictionary<IMorphologicalRule, int> _ruleCounts; | ||
| private readonly int _hashCode; | ||
|
|
||
| public AnalysisMergeKey(Word word) |
There was a problem hiding this comment.
Minor: F1. The merge key drops FinalTemplateState. #491 (b240b7b) added it to AnalysisStateKey, so the PR's "covering the same dimensions" is not accurate. Two analyses that differ only in that field used to stay separate. Now the second becomes an Alternative of the first. Every stratum output is cleared to None (L172-178), so this is probably sound, but Word.ValueEquals still compares the field. Please confirm it is intended and pin it with a test in AnalysisMergeKeyTests.
There was a problem hiding this comment.
F1 still applies at e723ade, and my earlier reason for calling it probably sound was wrong on this base. Stratum outputs are not cleared: FinalTemplateState is reset only on entry to a blocking stratum (AnalysisStratumRule.cs:135-136). So a merged NonTemplate canonical carries into a non-blocking stratum below, and AnalysisAffixTemplateRule.cs:47 then blocks a final template that the merged-away None alternative would have let through. The PR body still says the key covers the same dimensions as AnalysisStateKey. Unverified: this environment would not let me write a reproduction.
| @@ -606,255 +605,6 @@ public void ParseWord_SingleThreaded_MatchesParallel_WithCompounding() | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Minor: F2. The deleted ParseWord_MemoOnMatchesMemoOff_* tests compared maxDegreeOfParallelism: 1 against the parallel default. That still checks something real: the sequential CombinationRuleCascade added at AnalysisStratumRule.cs:51. Now only the compounding grammar checks that equivalence. Affix templates, a Linear stratum and Simultaneous epenthesis (buibui == 1) lose it, against docs/review/machine-tests.md:19. Keep them, renamed SingleThreaded_MatchesParallel_*, without the hit-counter asserts.
There was a problem hiding this comment.
F2 still applies at e723ade: the rewritten MorpherTests.cs still deletes the four ParseWord_MemoOnMatchesMemoOff_* tests. Only ParseWord_SingleThreaded_MatchesParallel_WithCompounding still compares sequential output with parallel output, so affix templates, the Linear stratum and Simultaneous epenthesis are no longer checked.
| // 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 |
There was a problem hiding this comment.
FYI: F4. Order invariance is no longer tested directly. The PR deleted AnalysisStateKeyTests.Equals_IsInvariantOverUnapplicationOrder_ForEqualMultisets, and order invariance is the property the XOR multiset hash exists for. A change to a rolling combine at AnalysisStratumRule.cs:272 would still pass both remaining tests and would quietly stop merging.
There was a problem hiding this comment.
F4 still applies at e723ade: AnalysisMergeKeyTests.cs has two tests, one for non-head count and one for unapplication count. Neither unapplies rules in a different order.
| _nonHeadCount = word.NonHeadCount; | ||
| _ruleCounts = word.UnappliedRuleCounts; | ||
|
|
||
| _shape.Freeze(); |
There was a problem hiding this comment.
FYI: F6. A plain constructor now changes its argument: it freezes word.SyntacticFeatureStruct, which Word.FreezeImpl leaves unfrozen. Before, the PinAndKey name and its doc comment said so. A one-line comment here would keep that warning.
There was a problem hiding this comment.
F6 still applies at e723ade: the AnalysisMergeKey constructor still freezes its argument's SyntacticFeatureStruct (AnalysisStratumRule.cs:255), and no comment says so.
|
Review: approve with fixes
API / framework / parity: Findings: F1 new; F2 new; F3 new; F4 new; F5 new, unverified; F6 new. Reviewed at 8313ba0 |
8313ba0 to
f3258c0
Compare
3a63132 to
df2cf21
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f3258c0 to
e723ade
Compare
Quick summary
Removes HermitCrab's opt-in analysis-cascade memo; the sequential rule cascade
and
MergeEquivalentAnalysesstill work exactly as before. Measured as apackage (#490), it cost 28-32% and ~29 GB on Sena for no net speedup on FLEx's
parallel path. Pruning (#474 landed; #491/#494/#519 open) is untouched and is
where the real parse-time wins come from.
Where to look
AnalysisMergeKey(AnalysisStratumRule.csL226-309) still distinguishes non-head count andper-rule unapplication counts -- pinned by
AnalysisMergeKeyTests.csandMergeEquivalentAnalysesTests.cs.AnalysisScope.cs,AnalysisStateKey.cs,MemoizedCombinationRuleCascade.csdeleted) --
maxDegreeOfParallelismand the sequential cascade are unchanged public API.docs/review/hermitcrab.mdand others).Validation
dotnet build(MSBUILDDISABLENODEREUSE=1) -- succeeded, 0 warnings, 0 errors.dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests --no-build-- 89 passed, 0 failed.dotnet csharpier check .-- 713 files checked, clean.pwsh -NoProfile -File scripts/comment-hygiene.ps1 -BaseRef origin/master-- clean.git diff origin/master..HEAD --stat-- 16 files, +196/-1520; production diff unchanged, onlyAnalysisMergeKeyTests.csadded../local_check.shwas not run as such; the commands above cover its steps individually.Reading this a year from now
The memo only ran when
maxDegreeOfParallelism == 1and tracing was off. FLEx usesthe default parallelism, so production never executed it. #490's ledger measured
the optimization round as a package: no net speedup, and cross-word memo sharing
was 28-32% slower on Sena and cost ~29 GB of memory. The gains keep coming from
search-space pruning that doesn't need a replay cache: #474 (landed), and #491,
#494, and #519 (open at the time of writing). Those shrink the work instead of
caching it, and they apply on the parallel path FLEx actually uses. The memo was
~1,400 lines of subtle code (
ReplayOntografting, arrival-count bookkeeping)whose correctness depended on an unenforced key invariant (#490, "narrowing that
key breaks correctness with no test failure").
What stays, and why
Morpher(..., maxDegreeOfParallelism)parameter and the sequentialcascade. They are API and still useful without the memo.
MergeEquivalentAnalysesfix. It keyed onAnalysisStateKey; that keynow lives as an internal
AnalysisMergeKeyinAnalysisStratumRule, covering thesame dimensions (shape, stratum, syntactic and realizational feature structures,
non-head count, per-rule unapplication counts).
Prior measurements (not reproduced by this review round)
These are earlier author measurements from the memo-removal work, not commands run
as part of this pass's validation, and no reproducible artifact for them is checked
into the tree:
each, all with at least one parse: analysis multisets (gloss, allomorph identity,
root index, feature structures) were reported identical between master memo-off
and this branch in both default and sequential modes, 25/25 match.
than this branch's sequential mode (e.g. 3,286 ms memo-on vs 3,122 ms without it;
412 vs 350 ms; the rest within noise).
🤖 Generated with Claude Code
This change is