Skip to content

perf(hermitcrab): share trailing analysis template slot batches - #546

Open
johnml1135 wants to merge 4 commits into
masterfrom
perf/shared-template-slot-batches
Open

johnml1135 wants to merge 4 commits into
masterfrom
perf/shared-template-slot-batches

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Quick summary

With no alternative limit, sequential untraced HermitCrab analysis runs one slot batch for templates with identical trailing slots. Sharing requires the same morphological rule objects in the same order and the same optionality, while retaining each template's output order and first-seen words. Tracing, parallel walks, finite alternative limits, custom compiled templates, changed slot optionality, and grammars without shared tails use the original battery.

Where to look

  • Slot identity and ordered output: SharedWalkMatchesPerTemplateWalkInOrder compares both ordered HermitCrab Word equality and complete AnalysisStateKey sequences across four grammar arrangements and 2,904 input/state combinations.
  • Repeated work: UntracedBatteryRunsSharedSlotBatchOnce pins 2 batches -> 1, then verifies the next call executes again; actually disabling sharing fails with Expected: 1; But was: 2, and restoring it passes.
  • Fallback contracts: TracedAndParallelBatteriesKeepPerTemplateWalk, SharedWalkPreservesBatteryAlternativeLimit, AlternativeLimitStopsBeforeLaterTemplateAdmission, and ChangedSlotOptionalityPreservesPerTemplateWalk cover tracing, parallelism, early limits, and mutable optionality. The latter two reproduced three failures before the compatibility guards were added.
  • Memo and folding: AnalysisStratumRule.ApplyTemplateBattery still wraps the same battery boundary; the full HermitCrab suite covers the existing memo and merge-equivalent analysis contracts.

Deliberately not included

Parallel sharing needs a separate design for ParallelApplySlots' layered breadth-first queue. This change shares the sequential depth-first walk; the existing parallel implementation remains in use. Corpus measurements are deterministic samples with capped pairs excluded; full-corpus parity and CI coverage remain unverified.

Validation

Commands ran with WSL ~/.dotnet, existing NuGet caches, package sources cleared by an uncommitted temporary NuGet.Config, and bounded worker counts. No network was used and the config is excluded from the commit.

  • dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release --no-restore -m:1 -- 126 passed, 0 failed, 0 skipped in the summary; the existing explicit corpus test is not selected by default.
  • ./local_check.sh -- FAILED (build test): SentencePiece's native build cannot find cmake (MSB3073, exit 127). Restore and CSharpier passed; no steps were skipped.
  • ./local_check.sh --agent-strict -- FAILED (build test) for the same missing CMake dependency; the blocking comment scan, restore, and CSharpier passed. These local runs do not collect CI coverage.
  • git diff --check aab237b2...HEAD -- clean.

The PR #480-head self-check runner was copied outside the worktree and built with a ProjectReference to this HermitCrab project (0 warnings/errors). Its loaded assembly hash matches this worktree's Release assembly. All 60 fixtures were attempted with --include-pathological, both normally and with --no-memoization; fixture contents and self-check assertions were unchanged. The no-memoization flag selects degree 0, exercising the original parallel path.

Assembly Memo on Memo off
This branch 58 passed / 2 failed 58 passed / 2 failed
Unchanged base aab237b2 58 passed / 2 failed 59 passed / 1 failed

The requested 60/60 baseline could not be reproduced. Both assemblies fail traced-rule attribution in boundary-leftover-in-narrowing-lhs-strrep; deep-optional-affix-nesting also crosses its 15-second budget in three runs. The latter has no shared slot tails and uses the original walk. These conformance and native-build blockers remain for caller review.

Exact runner invocations, repeated against the unchanged base assembly:

dotnet <runner>/hc-conformance.dll --fixtures <PR #480 head>/conformance --include-pathological
dotnet <runner>/hc-conformance.dll --fixtures <PR #480 head>/conformance --include-pathological --no-memoization

Issue / porting context

This is new work relative to PR #490, not row 2's memo: that memo reduces battery invocations, while this change reduces repeated slot batches inside one invocation. The precedent is sillsdev/PanGloss ledger entry 095, docs/divergences/095-shared-template-slot-batches.md, and its stratum.rs slot trie.


Parity argument and measured evidence

Each trie edge represents the same ordered rules and optionality beneath the same preceding slots in last-slot-first walk order. Every admitted template below it therefore reaches the edge with the same word its own walk reaches; a slot batch depends on that word and the rules, without template identity. Recursing through batch outputs in their existing order, then taking the optional unchanged-word continuation, preserves each template's exact depth-first emission sequence and admits or loses no parses.

Every template has its own output set, and those sets are unioned in template order with the existing comparer. The same first-seen representatives and ordered words therefore reach downstream deduplication and merge-equivalent folding. Admissions remain per template. Positive limits use the original walk to retain early termination; slot count/optionality changes also fall back, preserving the existing mutable-slot behavior.

All intermediate words and outputs are local to one battery call; only topology, configuration snapshots, and compiled rules persist. AnalysisScope.TemplateMemo still avoids a battery on a hit and stores the same complete outputs on a miss. No memo keys, retained-word bounds, replay rules, or trace events change.

Single-threaded measurements compare the original per-template walk (sharing disabled) with sharing enabled, using the same final source and a new analysis scope for each parse. Temporary counters exist only in the output-directory build. Both modes use the existing memo and are warmed, with their order alternated per input.

Input Completed segmentable pairs Slot batches before -> after Fewer batches Parse time before -> after
shared-template-unconstrained-suffix (10 words x 100) 1,000 2,000 -> 1,000 50.0% 1.27s -> 1.05s
Sena (200 sampled rows) 172 47,072 -> 32,287 31.4% 20.07s -> 16.67s
Mbugwe (200 sampled rows) 139 72,467 -> 58,486 19.3% 58.43s -> 51.33s

All completed segmentable pairs have identical morpheme-ID/surface signature lists after ordinal sorting, including multiplicity: zero differences and zero errors. Sena additionally has 28 matching invalid-shape outcomes and no capped pairs; Mbugwe excludes 61 pairs that hit a two-second watchdog in either mode. Samples select 200 evenly spaced nonempty rows from each existing word list. No corpus words, parse examples, private-project data, or grammar contents are included here or copied into the worktree.

The fixture is the only PR #480 grammar with shared trailing slots, identified by a structural scan of rule order and optionality; one ten-word pass executes 20 -> 10 batches. Wall times sum only completed ParseWord calls, exclude loading/signature formatting, and were collected under concurrent local validation load with counters/watchdogs, so they are secondary figures.

🤖 Generated with Claude Code


This change is Reviewable

@codecov-commenter

codecov-commenter commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.70992% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.69%. Comparing base (2599204) to head (5ed55e0).

Files with missing lines Patch % Lines
...orphology.HermitCrab/AnalysisAffixTemplatesRule.cs 97.29% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   74.57%   74.69%   +0.12%     
==========================================
  Files         453      454       +1     
  Lines       38314    38431     +117     
  Branches     5280     5304      +24     
==========================================
+ Hits        28573    28707     +134     
+ Misses       8576     8567       -9     
+ Partials     1165     1157       -8     

☔ 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.

Comment thread src/SIL.Machine.Morphology.HermitCrab/AnalysisAffixTemplatesRule.cs Outdated
Comment thread src/SIL.Machine.Morphology.HermitCrab/AnalysisAffixTemplatesRule.cs Outdated
Comment thread src/SIL.Machine.Morphology.HermitCrab/AnalysisAffixTemplatesRule.cs Outdated
@claude

This comment has been minimized.

@claude

This comment has been minimized.

johnml1135 and others added 4 commits October 23, 2026 07:04
…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>
@johnml1135
johnml1135 force-pushed the perf/shared-template-slot-batches branch from d6f481e to 5ed55e0 Compare October 23, 2026 11:10
if (
!_hasSharedSlots
|| _morpher.TraceManager.IsTracing
|| _morpher.MaxDegreeOfParallelism != 1

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.

Minor: F4. A default Morpher never takes the shared walk: maxDegreeOfParallelism defaults to 0 (Morpher.cs:29), so this guard sends every call to base.Apply and only callers who pass degree 1 get fewer batches. The PR body defers parallel sharing but presents the speedup as general; say in the description that it needs degree 1, or say why the default should not be covered.

@claude

claude Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve with fixes. The branch was rebased onto Remove HermitCrab's analysis-cascade memo #522, so I re-reviewed the whole PR. The shared walk still matches the per-template walk in output order, optional-slot skips and per-template admission.
  2. Most important: F4. A Morpher built with the default maxDegreeOfParallelism = 0 never reaches the shared walk (src/SIL.Machine.Morphology.HermitCrab/AnalysisAffixTemplatesRule.cs:62), so the speedup only reaches callers who pass degree 1. Separately, after the rebase the PR body still cites AnalysisScope.TemplateMemo and AnalysisStratumRule.ApplyTemplateBattery. Remove HermitCrab's analysis-cascade memo #522 removed both, and a grep of src/SIL.Machine.Morphology.HermitCrab finds neither, so the "Memo and folding" bullet and the parity argument describe code that no longer exists.
  3. Critical 0, Important 1, Low 0 new.
  4. Ran dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release --filter FullyQualifiedName~AnalysisAffixTemplatesRuleTests: 11 passed, 0 failed. From gh pr checks 546: Build on ubuntu-22.04 and on windows-latest passed, and Create NuGet package is pending. The Comment hygiene checks passed, but they are advisory and do not stand in for --agent-strict.
  5. Not verified: the corpus parse-time and batch-count figures, which came from temporary counters that are not in the branch, and whether those figures still hold on the rebased base without the Remove HermitCrab's analysis-cascade memo #522 memo.

Public API, target framework, package, machine.py parity: None verified. AnalysisAffixTemplatesRule and CanApply are internal.

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

Reviewed at 5ed55e0

johnml1135 added a commit that referenced this pull request Oct 23, 2026
…round

AnalysisScope.TemplateMemo no longer wraps the battery on master, so the statement that #546 composes with it is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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