Repository navigation
perf(hermitcrab): keep the required-slot look-ahead testing behind slots that write back deleted stem segments - #580
Conversation
…es a segment, allowing that many missing segments if the slot can restore them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## perf/hc-impure-slot-lookahead #580 +/- ##
=================================================================
+ Coverage 74.73% 74.78% +0.04%
=================================================================
Files 454 454
Lines 38730 38818 +88
Branches 5374 5398 +24
=================================================================
+ Hits 28946 29031 +85
- Misses 8603 8604 +1
- Partials 1181 1183 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// the slots before it are tested with those segments allowed to be missing from the shape. | ||
| /// </summary> | ||
| public bool IsPure(int slot) | ||
| public bool CanWalkPast(int slot) |
There was a problem hiding this comment.
FYI: F1. The class remarks still give the old soundness argument, which this change makes false: RequiredSlotMaterial.cs:82-87 says GenerateShape "never untruncates a segment" and that the test for slot j "is used only when every rule in the slots still to be walked after j is pure". CanWalkPast now walks past suppliers. Add a Supply paragraph (budget, cover, why suppliers force the unordered test) so the next change to Embeds does not rely on the old invariant.
| } | ||
|
|
||
| [Test] | ||
| public void Apply_OnEveryTruncationTemplateAndWord_KeepsTheParsesAndNeverViolatesTheShadow() |
There was a problem hiding this comment.
FYI: F2. The parallel path is never run past a supplier: this differential test builds its Morpher with one worker (:107), so it skips ParallelApplySlots, which also calls RequiredSlotCannotUnapply. The sibling AnalysisAffixTemplateFusionTests.cs:187-189 runs [TestCase(1)] and [TestCase(0)], and docs/review/hermitcrab.md asks for both paths. Add the same parameter here.
| if (child is Constraint<Word, ShapeNode> constraint && constraint.Type() == HCFeatureSystem.Segment) | ||
| { | ||
| FeatureStruct copy = null; | ||
| if (!constraint.FeatureStruct.HasVariables) |
There was a problem hiding this comment.
FYI: F3. Nothing tests the variable branch: a deleted constraint with variables adds a null supply, which CoverMasks treats as covering any constraint. I searched AnalysisAffixTemplateSupplyTests.cs for "Variable" and "alpha" and found nothing. If null were treated as covering nothing, alpha-feature truncations would be pruned and no test would fail. Add one truncating slot whose deleted segment carries a variable.
|
Verdict: approve. I found no case where the look-ahead prunes a valid parse. Most important: F1, the class remarks at Counts: 0 Critical, 0 Important, 3 Low (F1, F2, F3). Ran: Not verified: the parallel path past a supplier (F2), the variable supply branch (F3), and the Aweti and stress measurements in the description, which I did not rerun. Public API, target framework, package, machine.py parity: None verified changed. Findings: F1 new, F2 new, F3 new. Reviewed at db02d7e |
|
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. |
Start here:
src/SIL.Machine.Morphology.HermitCrab/RequiredSlotMaterial.cs, the per-slotBudget/SupplyandCoverMasks. 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
GenerateShapeonly copies input nodes and writes at mostBudgetnodes 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.Untruncatewriting 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
RequiredSlotLookaheadOff/Shadow/On switch. It follows #567 (R3); no separate issue. Nothing is cached and noInterlockedis added. Per slot, construction computes once the set of slots whose supply unifies with each allomorph constraint; during a walkSlotWalk.Suppliersis the mask of such slots walked so far.Decisions and why
RestoredSegmentsover its uncopied parts, the same countUntruncatewrites); one slot is one non-iterative pass per template invocation, so it unapplies at most once. A quantifier countsMaxOccurtimes its children; unbounded writes nothing.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.
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: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