Skip to content

perf(hermitcrab): run the required-slot look-ahead on the shared trailing-slot template walk - #577

Draft
johnml1135 wants to merge 8 commits into
masterfrom
perf/hc-lookahead-on-546
Draft

johnml1135 wants to merge 8 commits into
masterfrom
perf/hc-lookahead-on-546

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Start here: src/SIL.Machine.Morphology.HermitCrab/AnalysisAffixTemplatesRule.cs, the Walk method -- the per-template look-ahead mask on the shared trie walk. Review only the top commit (fcb65d7); the branch also carries #546 and #575.

Status: draft, stacked on #546 (shared trailing-slot trie, open) and #575 (required-slot look-ahead, open). Its base is master, so the PR diff also shows both. CI green. Closes #566.

What it does. The shared trailing-slot walk from #546 now runs #575's required-slot look-ahead at every trie node, for each admitted template below it. A template whose required slot cannot find its material is dead for that subtree while the templates sharing the node carry on; a child whose templates are all dead is skipped before its slot rule runs. On the benchmark grammars, adding the look-ahead to the shared walk cut a further 8.26% (Mbugwe), 25.97% (Sena) and 18.75% (Aweti) of work.

The unknown you arrive with: is it sound with sharing? Dead templates cannot complete (the #575 argument, applied per template), so outputs equal the shared walk without the look-ahead. The mask is one bool per template per call: no cache, no Interlocked. The same RequiredSlotLookahead Off/Shadow/On switch as #575 applies; shadow reports fired, violated and completed-under-fired counts per template.

Where to look

  • Pruning one template without losing its neighbour: LookaheadPrunesOnlyTheTemplateWhoseRequiredSlotIsGone.
  • No extra attempts when nothing is pruned: LookaheadKeepsEveryAttemptWhenBothTemplatesAreFeasible.
  • Shadow mode on the shared walk: ShadowLookaheadOnTheSharedWalkNeverViolatesOrChangesTheOutput.
  • Equality with the per-template walk on every input up to length 5: LookaheadSharedWalkMatchesPerTemplateWalkOnEveryShortInput.

Not here. The shared walk still runs only at Morpher.MaxDegreeOfParallelism == 1, as in #546; default callers get throughput from #582 (open). The look-ahead adds unification calls: 1.07% (Mbugwe) and 1.40% (Sena) more than #546 alone. Later look-ahead refinements are #576, #579 and #580.

Stack and verification. Merge order: #546, #575, then this. Ran earlier: HermitCrab test project, parity against master, #546, #575 and the combination on every benchmark word; shadow-mode violations 0. Not rerun after the rebase onto #522: the measurements below. The four truncate-morphotactic conformance failures are known.

Next: land #546 and #575, then review the top commit.


Reading this a year from now -- start here

This is the join of two independent optimisations: #546 removes repeated slot batches across templates, #575 prunes a partial parse as soon as a required slot's material is gone. Each alone leaves work the other removes, so the join is measured on its own (#566).

Evidence

Analysis non-template events plus synthesis steps, and unification calls including the look-ahead's own checks, master versus each variant. Measured before #522 removed the analysis memo; not rerun.

grammar #575 alone #546 alone both
Mbugwe 7.07% less / 0.33% less 26.05% less / 16.09% less 32.15% less / 15.19% less
Sena 34.30% less / 1.56% less 40.09% less / 6.13% less 55.64% less / 4.81% less
Aweti 7.90% less / 4.50% less 1.61% less / 1.14% less 20.06% less / 12.18% less
Amharic 9.10% less / 0.63% MORE 0.00% equal 9.10% less / 0.63% MORE
Indonesian equal equal equal

Added to #546: 8.26%, 25.97% and 18.75% less work on Mbugwe, Sena and Aweti, with 1.07% and 1.40% MORE unification calls on Mbugwe and Sena and 11.18% less on Aweti. Of the shared-slot work (about 65% of events before), 5.0% (Mbugwe), 20.2% (Sena) and 11.4% (Aweti) of events remain after both.

Shadow: 0 violations and 0 completions under a fired check on Mbugwe, Sena, Aweti, Amharic, Indonesian, conformance and the stress fixtures. Parses equal master, #546, #575 and the combination on every word.

Decisions and why

🤖 Generated with Claude Code


This change is Reviewable

johnml1135 added a commit that referenced this pull request Oct 28, 2026
Rows 43 and 44 move from in progress to done. R1 on #546 is draft PR #577
(commit bbb81a0, shadow 0 violations); R3 ordered look-ahead is draft
PR #576 (shadow 0 violations). Adds the round-doc section for both and
the PanGloss port issues (sillsdev/PanGloss #65, #66).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.55916% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.81%. Comparing base (c245476) to head (c8df81a).
⚠️ Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
...Morphology.HermitCrab/AnalysisAffixTemplateRule.cs 79.61% 16 Missing and 5 partials ⚠️
...hine.Morphology.HermitCrab/RequiredSlotMaterial.cs 90.00% 9 Missing and 6 partials ⚠️
...orphology.HermitCrab/AnalysisAffixTemplatesRule.cs 94.76% 7 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #577      +/-   ##
==========================================
+ Coverage   74.57%   74.81%   +0.24%     
==========================================
  Files         453      455       +2     
  Lines       38342    38752     +410     
  Branches     5284     5376      +92     
==========================================
+ Hits        28593    28993     +400     
- Misses       8583     8593      +10     
  Partials     1166     1166              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude

This comment has been minimized.

johnml1135 added a commit that referenced this pull request Oct 28, 2026
Adds the round-doc section "Combined stack and R2's cost": the full stack
against master (work, unification, matcher per grammar), additivity
(near-additive on Mbugwe, overlapping on Sena, super-additive on Aweti),
R2's root cause (its own probe, low prune rate) and the witness-reuse fix
folded into #572 @ 01bdf5a, the 1-4 point residual, and the landing
decision: #572 is a draft and lands only on top of #577 and #574. Stress
grammars are PR #578 (opt-in, [Explicit], not run in CI).

Ledger: row 40 (R2) Outcome records the fold and the landing decision;
new row 47 records the combined stack measurement.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135
johnml1135 requested a review from jtmaxwell3 October 28, 2026 18:08
johnml1135 and others added 6 commits October 28, 2026 21:42
…k guard, avoid per-node closures

The shared walk is only taken when MaxAlternatives is 0, so its limit check
could never fire; the guard comment now names all five fallback conditions;
Walk checks admitted templates with a loop instead of a capturing lambda.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#522 removed AnalysisStateKey along with the analysis-cascade memo and
kept the same identity as AnalysisStratumRule.AnalysisMergeKey, so the
shared-walk equivalence test now compares outputs under that key.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erial

An analysis template partial now stops as soon as a required slot can no
longer find the segments its affix inserts, instead of paying for every
optional slot before the required one. The test is used only while every slot
walked since is a pure copy-and-insert rule, so parses do not change. A
count-only shadow mode and a null-by-default check observer let a harness
verify soundness and cost on real grammars.

Closes #561

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… walk

Each template under a trie node gets its own look-ahead and a dead template stops collecting below it while its siblings go on.

Closes #566

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135

Copy link
Copy Markdown
Collaborator Author

Parked. This integrates #546 with the look-ahead; it will be re-cut as one commit once #546 and #575 have landed.

johnml1135 and others added 2 commits November 12, 2026 10:54
… pure optional slot, and the exhaustive test compares against look-ahead Off.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve. The skip rule is sound. After an optional slot is skipped, the parent's scan of the same word, starting at that slot, already covered every slot below a pure skipped slot, with the same fired, so leaving out the re-check cannot change the result.
  2. Most important: nothing new. F1 (src/SIL.Machine.Morphology.HermitCrab/AnalysisAffixTemplatesRule.cs:124) is fixed by the skipped flag, and F2 now compares against look-ahead Off.
  3. Counts: Critical 0, Important 0, Low 0 new.
  4. Ran: gh pr checks 577 shows both builds (ubuntu-22.04, windows-latest), the NuGet package, and comment hygiene passing; claude-review is pending. /code-review high found no correctness defect in the walk. Its other candidates are in Prune affix template partials whose required slots cannot find their material, in order and at the word edges #575 code, or are optimisations the diff gives no reason for.
  5. Not verified: I did not rerun the unification counts quoted in the F1 reply, did not run the new test against the old code, and did not check the grammar measurements. The branch was rebased since bbb81a00, so the compare reports diverged. As the PR asks, I scoped this round to fcb65d7b..c8df81ac; the perf(hermitcrab): share trailing analysis template slot batches #546 and Prune affix template partials whose required slots cannot find their material, in order and at the word edges #575 code it carries belongs to those reviews.

Public API, target framework, package, and sillsdev/machine.py parity: none verified changed. IsSlotPure and WalkState are internal/private, and HermitCrab has no Python counterpart.

Findings: F1 addressed; F2 addressed.

Reviewed at c8df81a

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HermitCrab analysis: measure required-slot look-ahead on the #546 shared walk

3 participants