Skip to content

perf(hermitcrab): keep the required-slot look-ahead testing behind slots that write back deleted stem segments - #580

Draft
johnml1135 wants to merge 2 commits into
perf/hc-required-slot-lookaheadfrom
perf/hc-supply-set-lookahead
Draft

johnml1135 wants to merge 2 commits into
perf/hc-required-slot-lookaheadfrom
perf/hc-supply-set-lookahead

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Start here: src/SIL.Machine.Morphology.HermitCrab/RequiredSlotMaterial.cs, the per-slot Budget/Supply and CoverMasks. Review only the top commit (db02d7e); earlier commits are #575, #576 and #579.

Status: open since 2026-10-28, stacked on #579 (open), which stacks on #576 (open), which stacks on #575 (open). CI green.

What it does. The required-slot look-ahead stopped at a slot it called impure. One kind blocks it on Aweti: a truncating allomorph, whose Lhs has a bare segment part the Rhs never copies, so unapplying it writes one segment back. On Aweti 21.80% of the non-template events sit behind such a slot. The walk now continues past a slot whose only impurity is writing segments back, and lets the required slots behind it be missing as many segments as the writing slots can restore. On Aweti that is 21.98% less work than #579.

The unknown you arrive with: can the prune ever drop a real parse? Not by construction. For these slots GenerateShape only copies input nodes and writes at most Budget nodes whose feature structures are the supply constraints. A required constraint unifying with no node in the shape can only be met by a written node, and each written node meets one constraint. More missing constraints than the covering slots can write means no parse. Budgets sum over distinct covering slots, and any Rhs action other than copy or insert stops the walk, as before.

Where to look

  • RequiredSlotMaterial: RestoredSegments, Budget/Supply, CoverMasks, the unordered presence test, Advance.
  • AnalysisAffixTemplateRule.RequiredSlotCannotUnapply, which now walks past suppliers.
  • AnalysisAffixTemplateSupplyTests: 8 methods, red when coverage is ignored, budgets are max instead of sum, or a modify-from action no longer stops the walk.
  • The relation rests on Untruncate writing only the Lhs constraint feature structures.

Not here, and the cost. The check is not free: on Aweti the look-ahead's own unifications rise 511.49% (232,639 to 1,422,564), now 9.2% of Aweti's unification calls, so the net 3.60% saving is thin. On the worst case (truncating-slot-chain, added in #578) unification calls are 5.12% MORE than #579. Aweti is the only real grammar affected.

Stack and verification. Merge order: #575, #576, #579, then this. Ran earlier: HermitCrab tests, csharpier, comment hygiene, shadow mode on Aweti and the worst case (0 violations). Not rerun after #522 removed the analysis memo; the measurements predate it.

Next: decide whether a 3.60% net unification saving on one grammar justifies the added code; approve or hold.


Reading this a year from now -- start here

Fourth commit of the look-ahead stack (#575, #576, #579, this), all under the RequiredSlotLookahead Off/Shadow/On switch. It follows #567 (R3); no separate issue. Nothing is cached and no Interlocked is added. Per slot, construction computes once the set of slots whose supply unifies with each allomorph constraint; during a walk SlotWalk.Suppliers is the mask of such slots walked so far.

Decisions and why
  • A budget is the most segments any one allomorph writes back (RestoredSegments over its uncopied parts, the same count Untruncate writes); one slot is one non-iterative pass per template invocation, so it unapplies at most once. A quantifier counts MaxOccur times its children; unbounded writes nothing.
  • Only slots whose allomorphs copy and insert, with slot index below 31, are suppliable. A slot's own write-back comes after its match, so it is excluded from its own coverage. Later invocations and strata re-run the check on their own entry shape.
  • A supply with variables covers everything, and a required constraint with variables is never counted missing; both only prune less.
  • Residual non-strictness is conservative: one existing node may satisfy several constraints, and nodes a walked slot later removes still count as found.
  • Three perf(hermitcrab): keep the required-slot look-ahead testing behind slots whose unmatched parts are unbounded runs #579 tests asserted "no pruning at entry" with a two-segment inner suffix; that is now pruned soundly, so they use a one-segment inner suffix the slot can restore.
Paths not taken

The worst-case grammar was not tuned. A first draft whose templates differed by a consonant pruned 60.03% of the work (the walk catches the wrong templates), so it was replaced by the vowel-deletion version, which is the worst case rather than the typical one.

Evidence

Per word, the same words in every arm; words aborted in any arm are dropped. Units: events plus synthesis steps, unification calls including the look-ahead's own checks, and matcher work. Measured before #522 removed the analysis memo; not rerun. "#579" is the base without the top commit.

grammar words events + steps: #579 / this unification calls: #579 / this matcher work: #579 / this
Aweti 99 11.50% less / 30.96% less 9.27% less / 12.53% less 11.67% less / 23.21% less
Mbugwe, Sena, Amharic, Indonesian, conformance 40 to 1,452 unchanged unchanged unchanged

Against #579 on Aweti: 21.98% less work (1,359,085 to 1,060,337 events plus steps), 3.60% less unification calls (16,059,467 to 15,481,951), 13.06% less matcher work (18,106,151 to 15,742,001). The saving in the rest (1,767,441 fewer calls) pays for the 1,189,925 extra check unifications.

Worst case truncating-slot-chain (60 words, 305 parses, 5 templates of 6 slots; required tense suffix, a required truncating suffix, three optional truncating suffixes of 10 rules each, a required subject prefix), against #579:

#579 this change
events + synthesis steps 27,005 26,765 0.89% less
unification calls incl. checks 384,433 404,111 5.12% MORE
matcher work 370,223 367,793 0.66% less
check-only unifications 8,305 29,963 260.78% MORE

Against master the same grammar is 3.96% less work, 5.66% MORE unification calls, 2.58% less matcher work. Shadow: 0 violations and 0 completions under a fired check on Aweti (9,837 fired) and the worst-case grammar (345 fired); the other grammars were 0 in the earlier run. Parses equal master on every completed word in the On and Shadow arms. The census counters live only on a census branch.

🤖 Generated with Claude Code


This change is Reviewable

@codecov-commenter

codecov-commenter commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.61%. Comparing base (d3c5c06) to head (a33bd60).

Additional details and impacted files
@@                         Coverage Diff                         @@
##           perf/hc-required-slot-lookahead     #580      +/-   ##
===================================================================
+ Coverage                            75.55%   75.61%   +0.05%     
===================================================================
  Files                                  455      455              
  Lines                                38915    39003      +88     
  Branches                              5424     5448      +24     
===================================================================
+ Hits                                 29404    29492      +88     
  Misses                                8364     8364              
  Partials                              1147     1147              

☔ 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
johnml1135 requested a review from jtmaxwell3 October 28, 2026 18:08
@johnml1135
johnml1135 marked this pull request as ready for review October 29, 2026 13:10
@johnml1135

Copy link
Copy Markdown
Collaborator Author

Ready for review. Counting every unit of work as equal, this does 9.1% less work on Aweti and leaves the other real grammars unchanged. Its worst case is a synthetic truncating-slot-chain grammar, where it does 2.2% more work (more unification calls, slightly fewer events and matcher steps).

Comment drafted by Claude (AI) for the maintainer.

@johnml1135

Copy link
Copy Markdown
Collaborator Author

Parked as a draft. The gain is on Aweti only (3.6% fewer unifications), so it waits until #575 lands and shows whether this tail is still worth the code.

johnml1135 and others added 2 commits November 12, 2026 10:57
…es a segment, allowing that many missing segments if the slot can restore them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…iable supply and the parallel path of the supply walk

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@johnml1135
johnml1135 force-pushed the perf/hc-supply-set-lookahead branch from db02d7e to a33bd60 Compare November 12, 2026 15:59
/// the shape lacks (<see cref="AnalysisMorphologicalTransform.GenerateShape"/> untruncates the segments of an
/// uncopied part). Such a slot is walked past, and is called a supplier, only when it copies and inserts and
/// nothing more, because a modified node could change any feature of any node. Its budget is the most
/// segments any of its allomorphs writes back, and its supply is the constraints of those segments; a constraint with variables

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F4. This comment line is 132 columns, over the 120-column limit that .claude/skills/code-comments/SKILL.md applies to /// blocks too. It was added with the Supply paragraph, and CSharpier does not rewrap comments. Rewrap the paragraph.


// The most segments the allomorph writes back, their constraints added to supply (null with variables).
// -1 if it does more than copy and insert, as a modified node can change any feature of any node.
private static int Restored(AffixProcessAllomorph allomorph, List<FeatureStruct> supply)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F5. Restored repeats the scan in IsPure (:512-531) line for line, and an allomorph is pure exactly when Restored returns 0. If the two drift apart, a slot that writes segments back could be treated as pure. Derive purity from Restored, and call RestoredSegments(part) once per part.

foreach (AffixTemplate template in new[] { consonantSlot, vowelSlot })
{
(WalkResult off, WalkResult on) = WalkBoth(template, "sagz");
Assert.That(on.RuleAttempts, Is.GreaterThan(0), "the variable supply may cover the missing segment");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F6. Weak assertion: this test discards off and only checks on.RuleAttempts > 0, so it still passes if pruning cuts the walk after the first attempts. Assert on.RuleAttempts == off.RuleAttempts, as the sibling tests at :256 and :277 do.

@claude

claude Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Verdict: approve. The supply walk is sound as far as I traced it: a supplier is walked past only when it copies and inserts and nothing more, and its budget counts the same segments that GenerateShape writes back. All three new findings are Low.

Most important: F5, RequiredSlotMaterial.cs:535. Restored duplicates IsPure, and the soundness of pruning depends on the two agreeing.

Counts: 0 Critical, 0 Important, 3 Low new (F4, F5, F6). F1 to F3 are addressed in a33bd60 and resolved.

Ran: /code-review high 580 over the whole PR, because the branch was rebased (compare db02d7e...a33bd60 reports diverged), then checked each candidate against the head. gh pr checks 580: Build on ubuntu-22.04 and windows-latest pass, Comment hygiene pass (advisory only, so it does not show the strict scan passed; F4 would fail it), NuGet package pending.

Not verified: that the stronger assertion proposed in F6 holds at the head, and the Aweti and stress measurements in the description.

Public API, target framework, package, machine.py parity: None verified changed. RequiredSlotMaterial is internal.

Findings: F1 addressed, F2 addressed, F3 addressed, F4 new, F5 new, F6 new.

Reviewed at a33bd60

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.

3 participants