Skip to content

Gate the compatibility normalizer on a streaming scan instead of a full parse - #626

Merged
JSv4 merged 2 commits into
mainfrom
perf/619-mc-normalizer-streaming-gate
Aug 31, 2026
Merged

Gate the compatibility normalizer on a streaming scan instead of a full parse#626
JSv4 merged 2 commits into
mainfrom
perf/619-mc-normalizer-streaming-gate

Conversation

@JSv4

@JSv4 JSv4 commented Aug 31, 2026

Copy link
Copy Markdown
Owner

Closes #619.

The gate that never closed

MarkupCompatibilityNormalizer.Normalize decided whether a part needed either of its two repairs by building the part's whole XDocument and sweeping it twice. What was supposed to keep that rare was a literal substring test:

if (!text.Contains("AlternateContent", StringComparison.Ordinal) &&
    !text.Contains("pPr", StringComparison.Ordinal))
    continue;

Every real word/document.xml contains pPr, so the gate never closed. PreAccept runs the normalizer once per side of every comparison, so that was a DOM build plus two descendant sweeps per side — and the pPr half exists for a repair that fires on paragraphs carrying two or more direct w:pPr, a malformed shape the reference document has zero of. The cost was almost entirely the cost of proving there was nothing to do.

What replaces it

A streaming XmlReader pass that answers the same two questions without building a tree: is there an mc:AlternateContent anywhere, and is there a paragraph with two or more direct pPr children. It tracks the open-element stack per depth, so a duplicate separated by a nested subtree (the shape a comparer produces when it inserts revision runs between the two malformed property elements) still pairs, and one pPr per sibling paragraph is never mistaken for a duplicate.

Two smaller things fall out of restructuring it as detect-then-rewrite:

  • The detection pass opens the archive read-only, so a package needing no repair never pays ZipArchiveMode.Update's entry buffering. That was option 3 in the issue; it comes free here rather than as a separate change.
  • The detection pass reads each entry as a stream rather than materialising it as a string, so a 574 KB document.xml is no longer decoded to a string just to be substring-searched.

Only a package with a candidate part reaches the rewrite pass, and only its candidate parts are parsed.

The gate is still a superset

The issue's constraint is the one that matters: a part that needs repair and is skipped is a correctness bug, where an over-broad gate is only a performance one. The scan matches on local names, while both repairs are namespace-exact (doc.Descendants(W.p), paragraph.Elements(W.pPr)). So the gate admits strictly more than the repairs act on — a false positive costs one parse of one part and still returns it unchanged, and a false negative cannot happen. That also preserves the property the old literal had and its comment cared about: a part binding the WordprocessingML namespace to a prefix other than w is still found.

Malformed XML answers "no", which is what NormalizePart already concluded by returning null on XmlException.

Measured

benchmarks/docxdiff-stress --probe against TestFiles/NVCA-Model-COI.docx (574 KB of document.xml, 15,360 elements), median of five:

before after
MarkupCompatibilityNormalizer.Normalize ×2 31.6 ms 10.2 ms
StrictOoxmlNormalizer.NormalizeToTransitional ×2 7.8 ms 7.9 ms

About 21 ms off every comparison, or roughly 6% of a ~350 ms Compare.

Output parity

The full corpus differential, which is the check this change actually needs — --probe alone would not catch a gate that wrongly closed:

$ dotnet run -c Release --project benchmarks/docxdiff-stress -- --corpus TestFiles --check main.json
[parity] OK - all 14,916 digests identical across 678 documents

That covers the redline package, revision list, edit script, all four consolidate products and the compatibility report, across eight generated edit shapes, for every .docx/.docm/.dotx in TestFiles/ — including the ones that do carry mc:AlternateContent.

Tests

Five new tests, all specifically about the gate rather than the repairs, and the issue is explicit that this is what was missing (the reference document exercises only the skip path):

  • Gate_FindsDuplicateParagraphPropertiesSeparatedByNestedContent — a whole w:ins subtree between the two duplicates.
  • Gate_FindsDuplicateParagraphPropertiesInsideANestedParagraph — the duplicate is in a text-box paragraph nested inside a well-formed outer paragraph.
  • Gate_DoesNotMistakeOnePPrPerSiblingParagraphForADuplicate — three sibling paragraphs, three pPr, no duplicate; counting per part rather than per paragraph would read that as a candidate.
  • Gate_ResolvesAlternateContentInAPartOtherThanTheMainDocument — every .xml entry is a candidate, not just word/document.xml.
  • Gate_FindsDuplicatesUnderANonstandardNamespacePrefix — the superset property the old literal had.

Verified non-vacuous: disabling the pPr half of the detector fails three of the five plus the pre-existing DisjointDuplicateParagraphProperties_AreCoalescedAndOrdered.

Full suite: 4180 passed, 0 failed, 3 skipped. No new files, so both warning baselines are unmoved.

JSv4 added 2 commits August 30, 2026 23:17
…f a full parse

Deciding whether a part needs either repair meant building its whole XDocument,
gated on a literal substring test for "AlternateContent" or "pPr". Every real
word/document.xml contains "pPr", so the gate never closed, and PreAccept runs
the normalizer once per side of every comparison -- a DOM build plus two
descendant sweeps per side, spent proving there was nothing to do.

A streaming XmlReader pass now answers the same two questions without building a
tree: any mc:AlternateContent, and any paragraph with two or more DIRECT pPr
children, tracked through the open-element stack so a duplicate separated by a
nested subtree still pairs and one pPr per sibling paragraph does not. The
archive is opened read-only for that pass, so a package needing no repair never
pays ZipArchiveMode.Update's entry buffering either; only a package with a
candidate part reaches the rewrite pass, and only its candidate parts are parsed.

The gate matches on local names where the repairs are namespace-exact, so it
stays a superset: a false positive costs one parse of one part and returns it
unchanged, and a false negative -- a part that needs repair and is skipped, which
would be a correctness bug -- cannot happen.

Normalize for both sides of a comparison on the NVCA model certificate of
incorporation: 31.6 -> 10.2 ms.

Closes #619
@JSv4
JSv4 merged commit b289d0e into main Aug 31, 2026
14 checks passed
@JSv4
JSv4 deleted the perf/619-mc-normalizer-streaming-gate branch August 31, 2026 05:06
JSv4 pushed a commit that referenced this pull request Aug 31, 2026
#625 fixed the defect this demo found (#614): InsertFootnote under
render_inline recording wrote the citation and the note definition as
ordinary content, so reject-all removed neither and the note's text
survived a "full" rejection. Act II had been carrying a plain paragraph
in place of the footnote it wanted, with a comment and a guard test
pinning the workaround. Both come out.

The marker goes at the end of the clause rather than against the figure
it qualifies, and that is the engine's constraint, not a style choice.
The "1.5x" text was written moments earlier as a tracked insertion, and
citing into it nests a reference run inside a w:ins — a revision with no
independently meaningful resolution. InsertFootnote refuses that outright
("offset falls inside a revision or unsupported inline container"), which
is the right answer and cost one iteration to discover. So the script
searches the clause's untouched tail and cites past it, which is also
where Word convention puts a marker.

That needed an offset the script does not know up front, so a step can
now bind one: `bindOffset` stores the character just past a search
match, and `offsetAfterMatch` is the arithmetic. It keeps the demo's
rule that an agent looks everything up rather than hardcoding it — a
literal offset would drift the moment any earlier act changed the
sentence.

The guard test is inverted rather than deleted: the script must still
contain exactly one note-insertion beat, because the reversibility proof
running over a footnote on every page load is the end-to-end check that
#625 works through the wire, and a silent removal would take it away.
A new browser assertion reads the citing paragraph's XML out of the
redline and requires the footnoteReference to sit inside a w:ins, so the
proof cannot pass vacuously by never writing a note; the reject-all test
gained the matching negative, that the side-letter text is gone.

Verified: the proof panel reads REVERSIBLE with zero content
differences, 26 revisions across 3 authors, 31 tool calls at a 5.1 ms
median. 14/14 browser assertions and 61/61 node checks pass.

Also retires the known-gap section in tracked_changes.md, which is now
false — it points at the encoding docs #625 added and keeps only the
account of how the closed-set classification surfaced it.

Republished figures after #626. Measured the same way the table is read,
three full 40-frame runs per depth on one container: revisions 152 ms
(was 144, inside the documented noise band), redline 181 ms (was 206),
full+HTML 352 ms (was 422). The ratio against the mutation path is now
60x to 154x, so the loop runs between 3 and 7 fps rather than 2 and 6.
The compareProducts-vs-two-calls pair, measured the controlled way
(fixed inputs, medians of nine, three reps), is 126 ms against 196 ms.
The two deeper rows moving 10-18% while the shallow one held is what a
load-path change looks like, though the panel measures the total, not
the attribution.
JSv4 pushed a commit that referenced this pull request Aug 31, 2026
The theater GIF predated the footnote beat, so it showed Act II inserting
a plain paragraph — a step that no longer exists. The new capture shows
the footnote rendering at the foot of the document and the proof panel
reading REVERSIBLE over it, which is the whole point of putting the beat
back. 31 calls, 26 revisions, 4.9 ms median.

The stress GIF predated #616 and #626 and read slower than the engine now
runs. Recut on the current build.

Its meter still reads higher than the table in the README (236 ms against
a published 181 ms for the same depth), and that gap is real rather than
a mistake in either: the capture runs a 10fps screenshot recorder against
the same CPU, and the clip starts after a full negotiation has already
grown the document. Both are the confound the README already describes,
so it now says so at the GIF rather than leaving a reader to find a
contradiction and trust neither number.
JSv4 pushed a commit that referenced this pull request Aug 31, 2026
Third engine movement this branch has tracked, and the first where all
three depths moved together: revisions 152 -> 124 ms, redline 181 -> 157,
full+HTML 307 from 352, each a median of three 40-frame runs. Ratio
against the mutation path is 64x to 165x; the loop runs 3 to 8 fps. The
controlled compareProducts pair (fixed inputs, medians of nine) is 106 ms
against 170 ms.

A saving uniform across depths is the signature of a change on the path
every depth shares, which is what #627 is -- it stopped giving the diff
engine's reads an identity nothing asks for. That reads differently from
#626, whose saving grew with pipeline depth, as a load-path change does.
The README now says both, and says plainly that the shape of the movement
is what suggests the attribution while the commits are the authority on
it; the panel only ever measures the total.

#629 landed in the same merge and is deliberately NOT credited. Its
snapshot reuse is across comparisons, and the stress loop makes one
comparison per frame against a document that changed, so there is nothing
for it to reuse. Crediting it because it arrived at the same time would
be the same mistake as reading a regression off the stress p50.

Also resolves the CHANGELOG conflict from the merge, keeping both
Unreleased entries -- #617's snapshot feature and the demo -- neither
supersedes the other.

Verified on the merged engine: 14/14 browser assertions, 61/61 node
checks. #629 changed docxodus_compare's catalog entry (adding mode and
outputPaths), which the contract test parses; additive, and the demo does
not call that tool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MarkupCompatibilityNormalizer full-parses document.xml on every call because the part contains the string "pPr"

1 participant