Speed up bulk Find & Replace with a native single-session string replace

[johnml1135]

Bulk Find & Replace over string fields (e.g. replacing text across thousands of Citation Forms) now runs each entry's search-and-replace through one native ICU search session instead of repeatedly restarting FindIn per match. Preview also computes each row's result once instead of twice. Both changes are additive: a new optional IVwPattern2.ReplaceAllIn COM capability, used when available, with the original repeated-FindIn path kept as the fallback when it isn't.

The diff crosses the native/managed boundary (a new COM interface) and touches a widely-used feature, so the real question isn't "is it faster" — the measurements below answer that — it's "does the fast path ever produce a different (or unsafe) result than the slow path did, and can a fault in it corrupt an in-progress bulk edit." That's what the checklist below is aimed at.

Where to look:

• COM/ABI safety — IVwPattern2 is a new GUID appended after the complete, unmodified IVwPattern vtable; verified against the generated MIDL header, not just the .idh source. No default-coclass change.
• Correctness across scripts/collation — VwPatternReplacementTests.cs cross-checks the real native ReplaceAllIn against a repeated-FindIn oracle: regex, collation/locale tailoring, whole-word, writing-system/style/tag runs, RTL, and combining marks.
• Fault safety mid-batch — a native fault during a real (non-preview) bulk apply used to risk leaving the outer undo task unterminated; now guarded with try/finally and pinned by Doit_OuterLoop_EndsUndoTaskWhenAnItemThrows.
• Fallback path — unchanged and covered (FakeDoit_FallsBackWhenBulkReplacementIsUnavailable); the capability is detected once, not probed per call.
• NFD-normalization skip check — hardened past the one existing Latin-diacritic case with a non-Latin (Hangul) multi-character decomposition, including inside a styled rich-text run.

Deliberately not here:

• Homograph-renumber maintenance, the next-largest remaining cost in a 100%-match bulk operation (~17%), is out of scope — it's liblcm-owned and needs its own cross-repo design. Filed as LT-22701.
• ReplaceAllIn searches a raw string and does not reproduce FindIn's VC-aware omission of embedded object-replacement characters (footnote markers etc.) from the pattern span — an architectural difference in what the two APIs search over, not a regression (see accordion).

Verification: ./build.ps1 (full native + managed): 0 warnings, 0 errors. Managed: 75/75 (ReplaceWithMethodPreviewTests, ReplaceAllInDecoratorCorrectnessTests, BulkEditBarTests, VwPatternReplacementTests). Native VwPattern suite: 27/27. The full 309-test native suite also reports all-pass but the process hangs ~5s in an unrelated Uniscribe/Graphite teardown path afterward (reproduced twice, unrelated subsystem, pre-existing). No manual FLEx UI pass was performed.

---

Reading this a year from now — start here

This PR started as a focused perf change (one squashed commit) and picked up a second commit from its own pre-merge review, which found and fixed three real issues before they shipped. The working measurement log that produced the perf numbers below lived at Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md on the branch; its conclusions are captured here and the file was deleted rather than merged, since it was a one-time investigation log, not guidance anyone needs to read to change this code correctly.

Decisions, and why

Why an optional IVwPattern2 capability instead of changing IVwPattern. IVwPattern is an existing, ABI-relied-upon COM interface. Adding ReplaceAllIn to it would have required every existing implementation and consumer to change in lockstep. Appending a new interface, detected once via an as/QueryInterface-style cast and cached, gets the perf win without touching the existing contract, and lets a caller that only implements IVwPattern keep working unmodified via the repeated-FindIn fallback.

Why preview and apply are independent evaluations, not a shared cache. FakeDoit (preview) and Doit (apply) are separate top-level calls, and the design intentionally recomputes on each — a bulk-edit column's preview row and its later apply are allowed to diverge if something else changed the underlying data in between. A per-row cache was added inside a single TryGetNewValue call (so BulkCopyMethod/TransduceMethod's OkToChange and TryGetNewValue share one computed value instead of two) but is explicitly cleared at the end of every call, so the preview-then-apply pair still each compute fresh. See "Reversals" below for what happens when that clearing is missing.

Why ReplaceAllIn doesn't omit embedded ORCs the way FindIn can. FindIn can search through a VwMappedTxtSrc, a view-constructor-aware text source that can skip owned object-replacement characters (e.g. footnote markers) so a pattern can match across one without "seeing" it. ReplaceAllIn operates on a raw ITsString via TrivialTextSrc and has no such view-aware skip. This wasn't something ReplaceAllIn's contract ever claimed to do; a bulk-replace call site that needs that omission would need to pass a pre-mapped source, which none currently does.

Reversals

The first version of the OkToChange/TryGetNewValue value-sharing cache (see above) did not clear itself between calls — it cached strictly by row ID, with no notion of "this call is done." That collapsed ReplaceWithMethod's intentional preview-then-apply double-evaluation down to a single evaluation, since both calls share the same row ID on the same method instance. It broke FakeDoit_MatchesImmediateApplyAcrossPatternModes (7 of its cases failed, each expecting the bulk-replace call count to reach 2, not 1) on the very next full test run after the "fix" was written. Caught by running the full suite rather than only the newly-added test, fixed by clearing the cache at the end of every TryGetNewValue call, and reverified at 75/75.

Deferred, and what would unblock it

Homograph-renumber batching (LT-22701): re-sorting and renumbering an entire homograph group happens on every single entry write, even though only the last write in a batch determines the group's final state — measured at roughly 34.6 microseconds/entry, about 17% of a 100%-match bulk operation. Unblocking it needs a liblcm-side design pass covering undo/redo interaction, PropChanged/notification behavior, cache membership mid-batch, and correct final numbering when multiple entries in the same group are edited in one operation — none of which this PR's scope (FieldWorks-side search/replace) can settle on its own.

Paths not taken

• A printable-ASCII memcmp shortcut in the native search path — reverted. ICU collation can equate strings that differ in punctuation and other non-ordinal ways, so a raw byte-compare shortcut produced wrong matches under real collation rules.
• A managed ordinal negative pre-check before invoking the pattern search — reverted; didn't produce a measurable benefit.
• A one-character collation-ignorable cache with an early extension exit — reverted. Its cached answer didn't survive a locale change and produced an incorrect match span; the single-character premise also had no ICU contract guaranteeing correctness under contextual collation.
• A virtual-table/budgeted coordinator design for spreading search cost — discarded as a test-owned prototype with no real activation path in product code.

Evidence

All measurements below are Debug-build, incremental (isolating one change at a time, not a cumulative branch-start-to-finish number), 50,000 real entries, and were taken before this PR's second (review-fix) commit — that commit fixes correctness/robustness issues and adds test coverage, and does not change the measured code paths.

Preview (single-pass) cost, 100% matching CitationForm, one match each: pooled median 72.4 → 51.4 microseconds/entry, a 29.1% reduction (1.41x throughput). Two paired process runs per build.

ReplaceAllIn apply cost (fresh processes, one warmup + five timed repetitions each, old old old old-######-style matched values with four matches each):

• 25% matching: 109.1 → 83.7 µs/entry, 23.2% reduction (1.30x).
• 50% matching: pooled median 234.1 → 171.6 µs/entry, 26.7% reduction (1.36x); the two paired runs individually showed 34.1% and 21.9% reductions.
• 100% matching: pooled median 448.5 → 326.1 µs/entry, 27.3% reduction (1.38x); paired runs showed 32.4% and 21.8%.

Debug-process variance was material — each independent 50%/100% pair exceeded a 15% self-imposed acceptance gate on its own, which is why the pooled-median figures above are quoted rather than any single run. Real-world gains should track the shape (bigger win at higher match rates) more reliably than the exact percentages.

Native test coverage added in the review-fix commit: testReplaceAllInReplaceCharPrecedingFinalORC_TE4727, testReplaceAllInRespectsMatchOldWritingSystem, testReplaceAllInWithCaseAndDiacriticsOptions, testReplaceAllInCanonicalEquivalence — re-running scenarios that previously only had FindIn coverage through the new bulk-session path. Full VwPattern native suite: 27/27 passing.

Preflight review details

Code Review Summary

Branch: table-speedup

Base: main (origin/main, merge-base 7f93348966a22be7fd4f9ef0c2e1cf571281cbcd)

Date: 2026-08-14

Review model: Claude Sonnet 5 (Claude Code)

Files changed: 9

Overview

This branch speeds up bulk Find & Replace over string fields. It adds an optional
IVwPattern2 COM capability (ReplaceAllIn) that replaces every match in a string
through one native ICU search session instead of the previous repeated-FindIn loop,
falls back to the old per-match path when the capability isn't available, and computes
each bulk-preview value once instead of twice. Measured gains (Debug, distillation doc):
~29% reduction in preview cost, ~23-27% reduction in ReplaceAllIn apply cost depending
on match rate.

Two independent specialist passes (native/COM/boundary-safety, managed C#/UI) reviewed
the diff. Both converged on the same real issue from different angles (undo-task safety
under a native fault), which was fixed and regression-tested during this review. Two
further findings were investigated and fixed (a narrower-than-claimed "compute once"
optimization, and thin NFD-normalization test coverage); one native test-coverage gap
was closed with new tests. All fixes were independently verified by full builds and test
runs, and one fix (the "compute once" caching) caught and corrected a real regression it
had itself introduced, verified before it reached this summary.

Contract/API Changes

IVwPattern2 (new GUID) adds one method, ReplaceAllIn, to the native Views COM
surface. Verified additive: the generated MIDL header shows IUnknown + the complete,
unmodified IVwPattern vtable + ReplaceAllIn appended last. The VwPattern coclass
lists both interfaces; QueryInterface handles both IIDs. No ABI break, no change to
the default coclass.

Findings

Critical - Must address before merge

None.

Important - Should address before merge

• BulkEditBar.cs's outer bulk-edit loop (Doit(IEnumerable<int>, ProgressState)) could leave an unterminated undo task if a native ReplaceAllIn fault occurred mid-batch during a real apply, since BeginUndoTask/EndUndoTask had no try/finally and the new bulk-replace path deliberately propagates exceptions rather than silently falling back. (fixed during review: wrapped the loop body in try/finally so EndUndoTask always runs; added Doit_OuterLoop_EndsUndoTaskWhenAnItemThrows, which fails without the fix and passes with it.)

Minor - Consider

• Native TestVwPattern.h's ReplaceAllIn tests didn't re-run the ORC, writing-system-restriction, case/diacritics, and NFD-equivalence scenarios that already existed as FindIn-only tests. (fixed during review: added testReplaceAllInReplaceCharPrecedingFinalORC_TE4727, testReplaceAllInRespectsMatchOldWritingSystem, testReplaceAllInWithCaseAndDiacriticsOptions, testReplaceAllInCanonicalEquivalence. All 27 VwPattern native tests pass, including the 4 new ones. One scenario — FindIn's "pattern spans an embedded, VC-omitted ORC" case — was deliberately not reproduced: ReplaceAllIn always searches through a raw TrivialTextSrc, which does not omit owned ORCs the way the VC-aware VwMappedTxtSrc used by that specific FindIn test does. This is an architectural difference in what ReplaceAllIn's contract covers (it operates on a plain ITsString, not a VC-mapped text source), not a bug; a TE4727-style adjacent-ORC scenario was used instead to still exercise real ORC-preservation in the bulk path.)
• The "compute preview once" win only reached ReplaceWithMethod; BulkCopyMethod and TransduceMethod still called NewValue twice per row in OkToChange and TryGetNewValue. (fixed during review: added DoItMethod.NewValueCached, a per-call cache that lets an OkToChange override share its computed value with TryGetNewValue instead of recomputing. The cache is cleared at the end of every TryGetNewValue call so a later, separate call for the same row — e.g. preview, then apply — still recomputes, since the underlying design intentionally treats those as independent evaluations. Added BulkCopy_ComputesSourceValueOnce. Self-caught regression: the first version of this cache did not clear between calls, which collapsed ReplaceWithMethod's intentional preview-then-apply double-evaluation down to one, breaking FakeDoit_MatchesImmediateApplyAcrossPatternModes (7 failures). Caught by rerunning the full suite, fixed, and reverified at 75/75 passing.)
• NormalizeResult's NFD-skip check (IsNormalized before calling get_NormalizedForm) was covered by only one Latin-diacritic test case (café). (fixed during review: added FakeDoit_NormalizesNonLatinScriptReplacementResultToNfd (Hangul syllable decomposition, a non-Latin script with a real multi-character canonical decomposition, unlike the single-diacritic Latin case) and FakeDoit_PreservesRichRunPropertiesWhenNormalizingNonLatinReplacementResult (a 1-character-to-3-character Hangul decomposition inside a styled, alternate-writing-system run, confirming run-property/offset-fixup survives a stronger decomposition than the existing café case). Both pass.)

Required Validation / Evidence

• ./build.ps1 (full native + managed) - 0 warnings, 0 errors.
• ./test.ps1 for xWorksTests (ReplaceWithMethodPreviewTests, ReplaceAllInDecoratorCorrectnessTests, BulkEditBarTests, VwPatternReplacementTests) - 75/75 passed against the final combined build.
• ./test.ps1 -SkipManaged -TestProject TestViews (native VwPattern suite, isolated via TestViews.exe -v VwPattern) - 27/27 passed, including the 4 new cross-coverage tests.
• ./test.ps1 -SkipManaged -TestProject TestViews (full native suite, twice) - both runs report Tests [Ok-Fail-Error]: [309-0-0] (all pass). Both runs then hang for ~5s during process teardown in an unrelated Uniscribe/Graphite rendering-engine subsystem (FindBreakPoint returned an error code), which test.ps1 reports as a failure after killing the hung process. This subsystem is not touched by this branch's diff, the hang is fully reproducible independent of any change here, and it occurs strictly after all tests report passing. Treated as a pre-existing environmental flake in the native test harness, not a regression from this branch.

Positive Observations

• Additive COM surface independently verified against the generated MIDL header (not just asserted from the .idh source).
• Native session state correctly moved from a per-call _alloca buffer to a persistent Vector<OLECHAR> member so it survives multiple NextAcceptedMatch calls across one bulk session.
• CheckedPatternPosition guards offset arithmetic against int overflow with an explicit failure instead of silent truncation.
• Managed VwPatternReplacementTests.cs cross-checks the real native ReplaceAllIn against a repeated-FindIn oracle across regex, collation/locale, whole-word, writing-system/style/tag-run, RTL, and combining-mark edge cases — genuine end-to-end integration coverage, not native-only or managed-only.
• The failure-propagation design (no silent fallback on a native fault) is deliberate and directly tested (FakeDoit_PropagatesBulkReplacementFailureWithoutFallback), not an oversight — the review's finding was specifically about the interaction with the outer undo-task wrapper, now fixed.
• Rejected/reverted experiments (an ASCII memcmp shortcut, an ordinal precheck, a collation-ignorable cache) are documented with why they were unsafe, in Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md, rather than silently dropped.

Interview Notes

• Author confirmed the undo-task gap should be fixed in this branch rather than deferred, since this branch is what makes the native call newly likely to fault mid-batch on odd per-row input; fixed and regression-tested as above.
• Author asked for the blast radius of fixing the BulkCopyMethod/TransduceMethod double-NewValue call before deciding whether to fix or just document; investigation found only two call sites (OkToChange overrides in those two classes), no external callers of OkToChange outside this file, and a fresh DoItMethod instance constructed per preview/apply phase (no cross-phase staleness risk) — low blast radius, so it was fixed rather than just documented.
• Author asked for broader NFD-assumption coverage rather than accepting the single existing café test, plus clear documentation of the change; both are reflected above and in the two new tests.
• Homograph-renumber batching (identified as the next-largest remaining cost during earlier characterization work on this branch, ~17% of a 100%-match bulk operation) was deliberately left out of this PR and filed separately as LT-22701, since it is liblcm-owned and needs its own cross-repository design pass covering undo/redo, notifications, and group-numbering correctness.

Suggested Review Focus

• Confirm the undo-task try/finally fix and its regression test match the team's expectations for how a mid-batch native fault during a real (non-preview) bulk apply should behave.
• Confirm comfort with leaving the ReplaceAllIn-vs-VwMappedTxtSrc ORC-omission architectural difference undocumented in code (noted here and in the PR) rather than adding a doc comment on IVwPattern2::ReplaceAllIn itself.

---

This change is 

[github-actions]

NUnit Tests

    1 files   -     1      1 suites   - 1   12m 39s ⏱️ - 1m 52s
5 822 tests +   61  5 741 ✅ +   99  81 💤 ± 0  0 ❌  - 38 
5 831 runs   - 5 709  5 750 ✅  - 5 554  81 💤  - 81  0 ❌  - 74 

Results for commit 30baeb4. ± Comparison against base commit 7f93348.

♻️ This comment has been updated with latest results.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 90.17094% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.10%. Comparing base (7f93348) to head (30baeb4).

Files with missing lines	Patch %	Lines
Src/views/VwPattern.cpp	90.30%	16 Missing ⚠️
Src/Common/Controls/XMLViews/BulkEditBar.cs	89.85%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1065      +/-   ##
==========================================
+ Coverage   37.91%   38.10%   +0.19%     
==========================================
  Files        1499     1499              
  Lines      350117   350276     +159     
  Branches    40233    40235       +2     
==========================================
+ Hits       132747   133480     +733     
+ Misses     188043   187521     -522     
+ Partials    29327    29275      -52     

Files with missing lines	Coverage Δ
Src/views/VwPattern.h	60.00% <ø> (+12.00%)	⬆️
Src/Common/Controls/XMLViews/BulkEditBar.cs	52.29% <89.85%> (+1.41%)	⬆️
Src/views/VwPattern.cpp	68.70% <90.30%> (+3.82%)	⬆️

... and 16 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.