Speed up bulk Find & Replace with a native single-session string replace - #1065
Speed up bulk Find & Replace with a native single-session string replace#1065johnml1135 wants to merge 5 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1065 +/- ##
==========================================
+ Coverage 38.35% 38.39% +0.04%
==========================================
Files 1507 1507
Lines 350617 350794 +177
Branches 40298 40306 +8
==========================================
+ Hits 134471 134696 +225
+ Misses 186916 186867 -49
- Partials 29230 29231 +1
🚀 New features to boost your workflow:
|
7cd1917 to
30baeb4
Compare
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
The ABI work here is done right, and I want to say that before the list. Versioning a
published COM surface by appending a new derived interface with its own GUID, adding it
to the coclass, extending QueryInterface with CSupportErrorInfo2, and detecting it
once by cast on the managed side is exactly how you extend Views without breaking
anyone — and verifying it against the generated MIDL header rather than the .idh
source is the right paranoia. The four reverted micro-optimizations, each documented with
why it was unsafe, are the most useful part of the body; the ICU-collation reasoning
behind dropping the memcmp shortcut is the kind of thing that saves the next person a
week.
I also checked the refactor's blast radius before writing this, and it is smaller than it
looks. Splitting FindInAlgorithmBase::Run and rerouting both forward Search branches
through SearchNext sits on the hot path of FwFindReplaceDlg and FindCollectorEnv,
which nothing in the verification list exercises directly — but the twenty existing
native tests in TestVwPattern.h (testSimpleSearch, testRealSearch,
testRegExpSearch, testMatchingWs, testSurrogatePairSearch, testORCSearch,
testReorderingDiactritics and the rest) all run through FindIn -> Run() ->
Search(), so the refactored path is pinned by construction. That is the coverage that
matters, and it is green. No action needed; I am recording it so nobody re-raises it.
What needs to change.
1. Roll back the batch on failure; do not commit it.
You correctly spotted a real bug: Doit(IEnumerable<int>, ProgressState) previously
called BeginUndoTask and EndUndoTask with nothing in between to guarantee the close,
so a throw left the undo task dangling open. That needed fixing.
But EndUndoTask() in a bare finally (BulkEditBar.cs:4839-4862) fixes it the weaker
way: when row 30,000 of 50,000 throws, the 29,999 already-applied rows are committed
as a completed undoable task and the exception then propagates. The user is left with a
half-applied bulk edit that undo treats as a finished operation.
This file has already answered this question four times —
UndoableUnitOfWorkHelper.Do(...) at BulkEditBar.cs:2147, :5976, :6469, :6999 —
and it rolls back on exception. FwFindReplaceDlg.cs:1211-1215 does the same explicitly,
setting undoHelper.RollBack = false only after the work succeeds. Please use the
helper.
The divergence is what I want to flag, more than the line itself. The reason this file
has one hand-rolled undo block and four helper calls is that each new piece of work in
here brings its own. That is the same drift that left the repo with two replace-all
engines (see item 3), and it is worth resisting on principle even where the behaviour
difference looks unlikely to bite.
Doit_OuterLoop_EndsUndoTaskWhenAnItemThrows
(BulkEditBarTests.cs:2536-2552) asserts only CurrentDepth == 0, which is true under
either policy — so it cannot tell commit from rollback and would not notice a
regression either way. Please assert the data: after the throw, the rows processed before
the failure are not present.
2. Take an IVwSearchKiller * on ReplaceAllIn now, even if every caller passes NULL.
Your own argument for creating IVwPattern2 rather than adding to IVwPattern is that a
published interface is permanent and changing it forces every implementation and consumer
to move in lockstep. That reasoning applies one level down. ReplaceAllIn hard-codes
NULL into both algorithm constructions (VwPattern.cpp:1367, :1373), so the entire
per-string replace is an uncancellable native call — and if cancellation is ever wanted,
the fix is IVwPattern3, for exactly the reason you gave.
This is not a regression: the old managed loop passed null too. It is a permanent shape
being decided by omission rather than on purpose, on a brand-new interface, while
UseRegularExpressions is reachable from the UI and catastrophic backtracking is a real
possibility on a 50,000-row table. An unused parameter costs one line today. A third
interface does not.
3. ReplaceAllIn breaks the COM out-parameter contract, and a new test asserts the
breakage.
VwPattern.cpp:1343-1353 validates pcMatches first, so a caller passing a null
pcMatches gets E_POINTER with *pptssResult left holding whatever it held on entry.
COM requires every [out] parameter be zeroed on failure.
testReplaceAllInValidatesOutputsAndRanges in TestVwPattern.h currently pins that:
unitpp::assert_true("result is untouched when first output is null",
ptssRawResult == reinterpret_cast<ITsString *>(1));
Every other failure case in that same test correctly asserts both outputs are cleared.
Validate all pointers before writing any of them, then let this case assert NULL like
its siblings.
4. Extract the zero-length-match fixup instead of copying it.
VwPattern.cpp:1291-1299 reproduces :1623-1638 character-for-character in logic — the
ichMin == ichLim test, the !m_fUseRegularExpressions || !m_stuCompiled.Equals(L"^")
guard, the +1 bump, the clamp. The original carries three explanatory comments and the
LT-6707 reference; the copy carries none. Two independent copies of a subtle Unicode
edge case will drift, and the commenting standard names LT-##### as the sanctioned
durable pointer precisely so this reasoning survives. A private helper on VwPattern
called from both sites fixes it.
5. Use ITsString.get_IsNormalizedForm.
BulkEditBar.cs:5188-5197 marshals tssResult.Text out as a BSTR to feed
CustomIcu.GetIcuNormalizer(...).IsNormalized(text). The interface you already hold
answers this directly — get_IsNormalizedForm(FwNormalizationMode), declared natively at
Src/views/lib/TsString.h:547 and used in this solution at
ConfiguredXHTMLGeneratorTests.cs:10436 and LcmWordGeneratorTests.cs:707. Reaching for
CustomIcu is correct SIL-library reuse in general; here a cheaper, closer API was in
hand.
While you are in there: the old code called get_NormalizedForm unconditionally, and the
new skip path returns the builder's string untouched when the text is already NFD. If
get_NormalizedForm also normalizes run segmentation, that changes run structure for
already-NFD input. FakeDoit_MatchesOracleForDecoratorBackedUnnormalizedRichValue
compares run-level equality against a get_NormalizedForm oracle, which is reassuring,
but it is one case.
6. Justify the numbers on a Release build, and narrow the regex claim.
The measured 29.1% / 23-27% figures are from a Debug build. For unoptimized C++ over ICU,
the ratio of per-call setup to per-match search cost is exactly what optimization changes
most, so Debug percentages are not a prediction of shipped behaviour. Please re-measure in
Release and quote those.
I am not asking you to commit the measurement log, and I am not asking for a durable
performance test — unless you can see one that would survive the Avalonia refactoring
intact. A benchmark fixture that gets rewritten or deleted in six months is worse than
none.
Separately, "one native ICU search session" is not true of the regex path.
RegexMatcher::find(start, status) resets the matcher and re-scans from start on every
call, so RegExFindInAlgorithm::SearchNext (VwPattern.cpp:1613-1625) still restarts per
match; what it saves there is the UnicodeString copy, the VwStringTextSource
construction and the FetchSearch round trip. Only the non-regex SearchNext
(:1583-1608), advancing m_piter->next(), is a genuine single session. Worth stating
precisely, because the structural argument for the win is strong on its own — preview
really does drop from two searches per row to one, and it is worth noting the probe you
removed was the cheaper of the two, which is why ~29% rather than ~50% is the honest
shape.
7. Test placement and the fixture that already covers this.
Both new managed files land in xWorksTests/Avalonia/Performance/. Nothing in either
touches Avalonia, and that tree otherwise holds only Composer/, Hosting/, Plugins/.
Please move them.
More substantively: xWorksTests/Search/BulkEditReplaceCharacterizationTests.cs already
exists, on the same base class, and its summary says it "Records the current preview and
apply behavior of bulk replacement over citation forms." That is the fixture whose entire
purpose is pinning the behaviour this PR changes. It is not in the verification list, and
the new preview tests were written from scratch in a new directory rather than extending
it. Either extend it or say why it is superseded — but a characterization fixture that
nobody consults during a semantic change is not doing its job.
8. VwPatternReplacementTests needs [Apartment(ApartmentState.STA)].
It calls VwPatternClass.Create() and VwStringTextSourceClass.Create(), both registered
threadingModel="Apartment". xWorksTests has no assembly-level apartment setting, and
the two sibling fixtures this PR adds in ReplaceWithMethodTests.cs both declare it. This
one is the odd fixture out.
9. Document the new null contract on NewValue.
TryGetNewValue returns newValue != null (BulkEditBar.cs:4912-4924), so null from
the abstract NewValue now means "skip this row" for every subclass. Today's four are
safe — BulkCopyMethod, TransduceMethod and ClearMethod all fall back to
TsStringUtils.EmptyString, and only ReplaceWithMethod returns null — so this is not
a live bug. But protected abstract ITsString NewValue(int hvo); carries no doc comment
saying so, and the next subclass author has no way to learn it.
10. Comments.
NewValueCached(BulkEditBar.cs:4926-4936) narrates its callers and their mechanism:
"reusing a value anOkToChangeoverride already computed ... The cache is cleared at
the end of everyTryGetNewValuecall." A member's summary states its own contract
only. One sentence does it: returnsNewValue(hvo), computing it at most once per hvo
until the cache is cleared.- The comment above
testReplaceAllInRespectsMatchOldWritingSysteminTestVwPattern.h
references another test by name, which the standard bans outright, and runs to roughly
235 characters against a 200-character budget. testReplaceAllInReplaceCharPrecedingFinalORC_TE4727(~228 chars) and the U+AC00
comment inReplaceWithMethodTests.cs(~215 chars) are both over budget; the first also
opens by restating the test's own name.// The first match extends past the end of our range.survives verbatim into
FindInAlgorithm::SearchNext(VwPattern.cpp:1601), where it is no longer the first
match.
The Views.idh block on IVwPattern2 is the model for the rest: four short lines,
contract only, and it states the non-obvious part ("Success leaves the pattern in a
terminal no-match state") that the tests then verify.
Compute each bulk preview value once. Replace all matches through one native ICU search session. Preserve rich text, Unicode collation, and legacy fallback behavior. Record measured gains and discarded experiments.
Guard the outer bulk-edit undo task with try/finally so a native ReplaceAllIn fault mid-batch cannot leave it unterminated. Share one computed value between OkToChange and TryGetNewValue in BulkCopyMethod and TransduceMethod instead of computing twice, scoped to stay correct across separate preview/apply calls. Add native ReplaceAllIn coverage for ORC, writing-system restriction, case/diacritics, and canonical equivalence. Harden the NFD-normalization skip check with non-Latin script and rich-run test cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Docs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md was a one-time investigation log, not durable guidance. Its measurements, rejected approaches, and follow-up items now live in the PR body. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wrap the bulk-edit apply loop in UndoableUnitOfWorkHelper.Do so a failure part way through rolls the whole batch back instead of committing the rows already applied. The old test asserted only that the undo depth returned to zero, which passes under either outcome, so it now succeeds on the first row and throws on the second and checks the first was rolled back. Add an IVwSearchKiller parameter to IVwPattern2::ReplaceAllIn while the interface is still unshipped, and thread it through both replacement algorithms instead of the hard-coded NULL. Clear both ReplaceAllIn out-parameters before rejecting a null argument so the COM contract still holds when E_POINTER comes back. Extract VwPattern::WidenZeroLengthMatch and call it from both search paths, carrying the LT-6707 explanation with it rather than duplicating the fixup. Use ITsString::get_IsNormalizedForm instead of marshalling a BSTR through CustomIcu to test whether the replacement result is already NFD. Move the bulk-replacement tests to xWorksTests/Search beside the characterization fixture, add the missing Apartment(STA) attribute, document the null return from NewValue, and bring five over-budget comments back under the hygiene limit.
Every case in BulkEditReplaceCharacterizationTests now runs through both the ReplaceAllIn path and the forced repeated-FindIn fallback, and the fixture requires identical text, run structure and writing system from both. That fixture already existed to record bulk replacement's preview and apply behaviour, so extending it is a better answer than writing new tests beside it, and it catches the failure mode this branch introduces: two engines that drift apart. Add a second normalization case where a run boundary falls between a base character and its combining mark in text that is already NFD. The skip path added here returns the builder's string untouched in that case, so run segmentation has to survive exactly as get_NormalizedForm would leave it, and one oracle case was thin evidence for that.
30baeb4 to
39c9122
Compare
|
Thank you — this review was accurate on all ten points, including the two you 1. Roll back the batch on failure. Agreed, and the divergence argument is the The old test could not tell commit from rollback, as you say. The replacement 2. Take an 3. COM out-parameter contract. Agreed — this was a real bug and the test was 4. Extract the zero-length-match fixup. Agreed. 5. On the run-segmentation question: you are right that one oracle case is thin 6. Release numbers, and the regex claim. Both fixed. Re-measured in Release on 50,000 entries. Rather than compare two builds I put three Preview, medians, microseconds/entry:
Apply:
On the apply path A and B are the same shape, because the pre-change The preview figure is the one the old measurement got wrong, and I would rather explain it Three caveats I would rather state than have you find:
On the regex claim: you are right, and the body now says it precisely, in the lead I took your constraint on the measurement seriously — no committed log, no 7. Test placement, and the fixture that already covered this. Both files are The second half of your point was the more useful one, and you were right that I 8. 9. Document the null contract on 10. Comments. All four fixed. Worth flagging: the Verification. Debug build clean, 0 warnings. |
3 participants
Bulk Find & Replace over string fields (e.g. replacing text across thousands of Citation Forms) now performs each entry's replacements inside the native layer instead of driving a managed loop that restarts
FindInper match -- a single ICU search session for a literal pattern, and for a regular expression a single native call per match rather than a single session (RegexMatcher::findre-scans from its start position every time). Preview also computes each row's result once instead of twice. Both changes are additive: a new optionalIVwPattern2.ReplaceAllInCOM capability, used when available, with the original repeated-FindInpath 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:
IVwPattern2is a new GUID appended after the complete, unmodifiedIVwPatternvtable; verified against the generated MIDL header, not just the.idhsource. No default-coclass change.VwPatternReplacementTests.cscross-checks the real nativeReplaceAllInagainst a repeated-FindInoracle: regex, collation/locale tailoring, whole-word, writing-system/style/tag runs, RTL, and combining marks.try/finallyand pinned byDoit_OuterLoop_EndsUndoTaskWhenAnItemThrows.FakeDoit_FallsBackWhenBulkReplacementIsUnavailable); the capability is detected once, not probed per call.Deliberately not here:
ReplaceAllInsearches a raw string and does not reproduceFindIn'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). NativeVwPatternsuite: 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.mdon 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
IVwPattern2capability instead of changingIVwPattern.IVwPatternis an existing, ABI-relied-upon COM interface. AddingReplaceAllInto it would have required every existing implementation and consumer to change in lockstep. Appending a new interface, detected once via anas/QueryInterface-style cast and cached, gets the perf win without touching the existing contract, and lets a caller that only implementsIVwPatternkeep working unmodified via the repeated-FindInfallback.Why preview and apply are independent evaluations, not a shared cache.
FakeDoit(preview) andDoit(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 singleTryGetNewValuecall (soBulkCopyMethod/TransduceMethod'sOkToChangeandTryGetNewValueshare 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
ReplaceAllIndoesn't omit embedded ORCs the wayFindIncan.FindIncan search through aVwMappedTxtSrc, 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.ReplaceAllInoperates on a rawITsStringviaTrivialTextSrcand has no such view-aware skip. This wasn't somethingReplaceAllIn'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/TryGetNewValuevalue-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 collapsedReplaceWithMethod'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 brokeFakeDoit_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 everyTryGetNewValuecall, 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
memcmpshortcut 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.Evidence
All measurements below are Release x64, 50,000 real entries, taken after this PR's
review-fix commits. The harness runs all three algorithm shapes in one process and
alternates them repetition by repetition, so machine drift lands on all three equally:
then
NewValuescans the row again through the repeated-FindInloop.repeated-
FindInloop.IVwPattern2::ReplaceAllIn.A -> Bis the "compute each preview value once" change in isolation;B -> CisReplaceAllInin isolation;A -> Cis what a user gets. Figures are medians of fivetimed repetitions after one warm-up of each arm, in microseconds per entry.
Preview pass
Apply pass
On the apply path arms A and B are the same shape, because the pre-change
Doit(int hvo)called the base enabled-gate on purpose to avoid the duplicate search. The
A -> Bcolumnis therefore a wiring check on the harness rather than a result: it reads -0.6% to 3.0%,
so the two arms are behaving identically and the whole apply win is
ReplaceAllIn. Thesethree figures are also the closest thing here to a cross-check on the old harness - it
reported 23.2% / 26.7% / 27.3% for the same three match rates, against 29.2% / 30.1% /
31.4% now.
Reading the preview numbers.
B -> Crises monotonically with the number of matchesper row (17.8% -> 27.8% -> 30.8% -> 33.5%), which is the expected signature of replacing a
per-match managed loop with one native session.
A -> Bdoes not move monotonically, and the reason is worth stating because it correctsthe intuition that the removed search was cheap. The pre-change enabled-gate searched
every previewed row, matching or not, before the match test - so its cost was a whole
extra ICU search setup per row, not a fraction of one. Taking the two 100%-matching
scenarios, where the saving is simply the difference of medians with no modelling:
removing it saves 49.2 microseconds/row on a 10-character citation form and 70.0
microseconds/row on a 22-character one. The percentage therefore depends on field length
and on how much other work the row needs, which is why it ranges from 14.8% to 41.0%
here rather than sitting near a single figure.
Caveats.
in this PR. That harness was a throwaway that no longer exists, its field content and
cache setup are unrecorded, and its pre-change absolutes differ from these by roughly 2x
in the direction Debug-vs-Release cannot explain. Only the within-run ratios above
should be read as results.
alternating design means load falls on all three arms, so the ratios hold; the absolutes
would be lower on a quiet machine.
[Category("ByHand")]fixture deliberately kept out of the branch, perthe review's request for no committed log and no durable benchmark fixture.
Native test coverage added during review:
testReplaceAllInReplaceCharPrecedingFinalORC_TE4727,testReplaceAllInRespectsMatchOldWritingSystem,testReplaceAllInWithCaseAndDiacriticsOptions,testReplaceAllInCanonicalEquivalence- re-running scenarios that previously had onlyFindIncoverage through the new bulk-session path. Full nativeTestViewssuite:309 passing, 0 failures.
Managed coverage: the characterization fixture now runs every case through both the
ReplaceAllInpath and the forcedFindInfallback and requires identical text, runstructure and writing system from both, so the two engines cannot drift apart unnoticed.
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
IVwPattern2COM capability (ReplaceAllIn) that performs every replacement in a stringinside the native layer instead of driving a managed repeated-
FindInloop, falls back tothe old per-match path when the capability isn't available, and computes each bulk-preview
value once instead of twice.
For a non-regex pattern that native replacement is a genuine single ICU search session,
advancing one iterator via
m_piter->next(). For a regular expression it is not:RegexMatcher::find(start, status)resets the matcher and re-scans fromstarton everycall, so the regex path still restarts per match, and what it saves there is the
UnicodeStringcopy, theVwStringTextSourceconstruction and theFetchSearchroundtrip.
Measured gains (Release, 50,000 entries, medians): preview cost down 39-52% depending on
match rate and field length, apply cost down 29-31%. Full method and per-scenario
breakdown, including which half of the change each figure belongs to, in Evidence below.
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 COMsurface. Verified additive: the generated MIDL header shows
IUnknown+ the complete,unmodified
IVwPatternvtable +ReplaceAllInappended last. TheVwPatterncoclasslists both interfaces;
QueryInterfacehandles both IIDs. No ABI break, no change tothe 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 nativeReplaceAllInfault occurred mid-batch during a real apply, sinceBeginUndoTask/EndUndoTaskhad notry/finallyand the new bulk-replace path deliberately propagates exceptions rather than silently falling back. (fixed during review: wrapped the loop body intry/finallysoEndUndoTaskalways runs; addedDoit_OuterLoop_EndsUndoTaskWhenAnItemThrows, which fails without the fix and passes with it.)Minor - Consider
Native(fixed during review: addedTestVwPattern.h'sReplaceAllIntests didn't re-run the ORC, writing-system-restriction, case/diacritics, and NFD-equivalence scenarios that already existed asFindIn-only tests.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:ReplaceAllInalways searches through a rawTrivialTextSrc, which does not omit owned ORCs the way the VC-awareVwMappedTxtSrcused by that specificFindIntest does. This is an architectural difference in whatReplaceAllIn's contract covers (it operates on a plainITsString, 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(fixed during review: addedReplaceWithMethod;BulkCopyMethodandTransduceMethodstill calledNewValuetwice per row inOkToChangeandTryGetNewValue.DoItMethod.NewValueCached, a per-call cache that lets anOkToChangeoverride share its computed value withTryGetNewValueinstead of recomputing. The cache is cleared at the end of everyTryGetNewValuecall 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. AddedBulkCopy_ComputesSourceValueOnce. Self-caught regression: the first version of this cache did not clear between calls, which collapsedReplaceWithMethod's intentional preview-then-apply double-evaluation down to one, breakingFakeDoit_MatchesImmediateApplyAcrossPatternModes(7 failures). Caught by rerunning the full suite, fixed, and reverified at 75/75 passing.)NormalizeResult's NFD-skip check (IsNormalizedbefore callingget_NormalizedForm) was covered by only one Latin-diacritic test case (café).FakeDoit_NormalizesNonLatinScriptReplacementResultToNfd(Hangul syllable decomposition, a non-Latin script with a real multi-character canonical decomposition, unlike the single-diacritic Latin case) andFakeDoit_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.ps1forxWorksTests(ReplaceWithMethodPreviewTests,ReplaceAllInDecoratorCorrectnessTests,BulkEditBarTests,VwPatternReplacementTests) - 75/75 passed against the final combined build../test.ps1 -SkipManaged -TestProject TestViews(nativeVwPatternsuite, isolated viaTestViews.exe -v VwPattern) - 27/27 passed, including the 4 new cross-coverage tests../test.ps1 -SkipManaged -TestProject TestViews(full native suite, twice) - both runs reportTests [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), whichtest.ps1reports 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
.idhsource)._allocabuffer to a persistentVector<OLECHAR>member so it survives multipleNextAcceptedMatchcalls across one bulk session.CheckedPatternPositionguards offset arithmetic againstintoverflow with an explicit failure instead of silent truncation.VwPatternReplacementTests.cscross-checks the real nativeReplaceAllInagainst a repeated-FindInoracle 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.FakeDoit_PropagatesBulkReplacementFailureWithoutFallback), not an oversight — the review's finding was specifically about the interaction with the outer undo-task wrapper, now fixed.memcmpshortcut, an ordinal precheck, a collation-ignorable cache) are documented with why they were unsafe, inDocs/superpowers/plans/evidence/bulk-replacement-distillation-2026-08-13.md, rather than silently dropped.Interview Notes
BulkCopyMethod/TransduceMethoddouble-NewValuecall before deciding whether to fix or just document; investigation found only two call sites (OkToChangeoverrides in those two classes), no external callers ofOkToChangeoutside this file, and a freshDoItMethodinstance constructed per preview/apply phase (no cross-phase staleness risk) — low blast radius, so it was fixed rather than just documented.Suggested Review Focus
try/finallyfix 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.ReplaceAllIn-vs-VwMappedTxtSrcORC-omission architectural difference undocumented in code (noted here and in the PR) rather than adding a doc comment onIVwPattern2::ReplaceAllInitself.This change is