Memoize HermitCrab's sequential analysis cascade - #456
Conversation
5c42eaa to
8f5438a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #456 +/- ##
==========================================
+ Coverage 73.31% 73.53% +0.21%
==========================================
Files 446 449 +3
Lines 37334 37633 +299
Branches 5121 5174 +53
==========================================
+ Hits 27371 27673 +302
+ Misses 8836 8824 -12
- Partials 1127 1136 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit made 1 comment.
Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on johnml1135).
a discussion (no related file):
Before I do a full review, it would be great if you could rebase this on master. I suspect that some of the logic might need to be updated to work with the changes in PR #452.
8f5438a to
70bff72
Compare
|
Rebased this branch onto current Decisions made during the rebase:
Verification on final commit
|
af80918 to
d9ccc1f
Compare
|
@ddaspit - it has been rebased and shouldn't have any issues. |
Adds AnalysisStateKey/AnalysisScope/Word.ReplayOnto and wires a memo table into both the mrule cascade and the affix-template battery, so repeated analysis-cascade states reached via different rule-unapplication orders are computed once and replayed rather than re-expanded. Off by default (the existing parallel cascade is unchanged); opt in via Morpher(maxDegreeOfParallelism: 1), which selects the new sequential+memo path. Ported from an archived prototype (parse-optimization-archive) with stronger verification: the acceptance gate is analysis-set equality (canonical morpheme-signature sets), not byte-identical objects, since a memo-replayed Word is not guaranteed field-for-field identical to a freshly-computed one. Verified via unit tests (key order-invariance, replay graft correctness, in-flight re-entry guard) plus corpus runs against three real grammars (Sena, Indonesian, Amharic) with zero analysis-set divergences. On a known-pathological word, sequential+memo measured 6.2x faster than the parallel default (isolated to ~6.3x attributable to the memo itself, not threading); aggregate corpus evidence and the typical-word tradeoff are in memoization.md, along with honestly-reported open gaps.
- Clear AnalysisScope entering synthesis, so returned parses no longer pin the per-parse memo tables - Make maxDegreeOfParallelism an enforced cap and retire the dead SINGLE_THREADED toggles it was meant to replace - Keep the template memo off Linear strata, whose key completeness is unaudited, and reject unfrozen words as memo keys - Deduplicate replay/store onto AnalysisScope, drop ReplayOnto's discarded clones, use plain collections on the sequential-only path, and match CombinationRuleCascade's expansion order - Make the diagnostic counters atomic; trim comments to non-obvious constraints and remove memoization.md Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
d09e405 to
e37c288
Compare
- Wire MaxDegreeOfParallelism through the Parallel.ForEach call sites that ignored it, so values above 1 actually cap concurrency - Bound the memo by retained Words, not just entry count - Skip the per-node result lists when no scope is installed - Replace process-global hit counters with per-scope counts folded into per-Morpher totals - Build keys via AnalysisStateKey.PinAndKey so the freeze side effect is visible at call sites; document InProgress as defensive Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 14 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on johnml1135).
Summary
new Morpher(traceManager, language, maxDegreeOfParallelism: 1), which selects the new sequential+memo path.Wordisn't guaranteed field-for-field identical to a freshly-computed one, even when it represents the same analysis.The design write-up (
memoization.md) that earlier revisions of this branch carried has been removed — it duplicated what the code now documents in place. The key-completeness audit lives onAnalysisStateKey, the graft's soundness argument onWord.ReplayOnto, and the memo's resource and re-entry rules onAnalysisScope. This PR description is now the only record of the corpus measurements below.Follow-up: #457 stacks a data-structure rearchitecture (array-backed, copy-on-write
Shape) on top of this branch and re-measures the same two heavy words — read that PR for whether it's worth the larger diff.Why off by default — the actual tradeoff
This is not a pure win, and the corpus evidence says so directly:
So flipping the library's default cascade mode trades typical-word latency for pathological-word latency. Whether that's the right tradeoff depends on a given corpus's word-difficulty distribution — a decision for a future PR with its own evidence, not this one.
None of these numbers have been re-measured since the review changes below, which are correctness/resource fixes rather than algorithmic ones.
maxDegreeOfParallelismThe new constructor parameter replaces the dead
SINGLE_THREADEDcompile-time toggles it was standing in for, and is now an enforced runtime cap rather than only a mode switch:1runs the parse fully sequentially and is the only configuration eligible for the memo.>= 2is passed to everyParallel.ForEacha parse or generation goes through — the mrule cascade, affix-template unapplication, synthesis, and word generation. Previously onlyParallelCombinationRuleCascadehonoured it, somaxDegreeOfParallelism: 2still saturated the machine.0(the default) and anything below 1 leave concurrency unbounded, preserving today's behaviour exactly. Call sites that already had a narrower default than TPL's unbounded —Synthesize's loop usedEnvironment.ProcessorCount— keep it.It 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.
Review changes
Two follow-up commits address review findings; neither changes the algorithm or the analyses produced.
Address review findings in analysis-cascade memo:AnalysisScopeentering synthesis, so returned parses no longer pin the per-parse memo tables.AnalysisScope, dropReplayOnto's discarded clones, and matchCombinationRuleCascade's expansion order.Tighten memo resource bounds, diagnostics, and parallelism cap:Parallel.ForEachcall sites that ignored it (above).Words, not just entry count. The entry cap alone was a poor proxy for memory: entry size is unbounded — a node's stored list holds every descendant, undeduplicated — and storing them keeps every intermediate of the search alive for the whole parse. Both tables share one per-parse budget. It is a coarse backstop, not a figure derived from measured memory, and should be re-derived from peak RSS on the heavy words before being treated as a real bound.Morphertotals. The old statics were only safe because this test assembly is not[Parallelizable]; the equivalence gates now assert on a freshMorpher's own counts, which is both stronger and immune to cross-test leakage.AnalysisStateKey.PinAndKeyso the freeze side effect on the caller'sWordis visible at every call site, and documentAnalysisScope.InProgressas defensive — no path reaches it today, since every unapplication grows the multiset the key hashes, so a key cannot recur while still on the stack.Verification
AnalysisStateKeyorder-invariance/hash/equality and its frozen-word precondition,Word.ReplayOntograft correctness (including a test that specifically distinguishes "grafted the right subtree" from "grafted the wrong one" via distinct lexical entries), theInProgressin-flight re-entry guard, positive-replay-vs-unmemoized-result-set equivalence (including trail order),MaxAlternativesenforcement on both the raw and replay paths, the template memo's Unordered-only gating (AnalysisStratumRuleTests), the parallelism-cap mapping, and an mrule/template equivalence battery against real analysis-rule content viaMorpherTests.Test plan
dotnet build Machine.slndotnet csharpier check .dotnet testacross HermitCrab, SIL.Machine, and Thot test projectsMemoCorpusVerification,[Explicit]) run manually against local Sena/Indonesian/Amharic grammars — 0 divergences on the words that completed within the timeout budgetThis change is