Repository navigation
Buffer consecutive text nodes when loading XContainer to avoid O(n^2) - #130835
jeffhandley merged 2 commits into
Conversation
…dotnet#37893) XContainer's IXmlSerializable/load path materialized each text node individually via AddStringSkipNotify, which concatenates immutable strings. When an XmlReader surfaces a single logical text value as many small consecutive text nodes (e.g. DataContractSerializer reading over a stream, or IXmlSerializable.ReadXml), this degrades to O(n^2) and makes deserialization pathologically slow. Buffer runs of adjacent text (Text/SignificantWhitespace/Whitespace) into a StringBuilder and materialize them in a single pass, keeping the load linear. Rather than sprinkling flush calls across every non-text branch, each of the four ContentReader read methods is split in two: a fast path that appends coalescable text to the buffer and returns, and a slow path that flushes the buffer once before switching over any non-text node. The buffer is also flushed at the end of each driving loop so trailing document-level text (e.g. whitespace after an XDocument's root element, where the reader stops at end-of-input rather than a matching EndElement) is preserved. The resulting node tree is identical to before: AddStringSkipNotify still coalesces consecutive strings, flushing always happens before the current container or base URI changes, and text requiring its own base URI or line info continues to fall through to a standalone XText. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d0f435a0-6043-4180-994f-7270b1edc8ac
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates System.Xml.Linq’s XContainer XML load path to avoid quadratic behavior when an XmlReader reports one logical text value as many consecutive small text nodes, by buffering coalescable text runs and materializing them in one pass. It also adds regression tests that emulate chunked text delivery (sync and async).
Changes:
- Buffer consecutive
Text/Whitespace/SignificantWhitespacenodes during load and flush before handling non-text nodes, plus a final flush when the driving loop ends. - Extend load behavior to flush trailing buffered text on end-of-input for both sync and async load paths.
- Add tests (including a chunking
XmlReader) to validate coalescing and guard against algorithmic regressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XContainer.cs | Adds text-run buffering and explicit buffer flush points in the load/read loops (sync + async, with/without base URI/line info options). |
| src/libraries/System.Private.Xml.Linq/tests/misc/RegressionTests.cs | Adds regression coverage for chunked text loading (sync/async) and mixed content, plus a large-input linearity guard. |
…ecutive text node, and upgrading to StringBuilder for more.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
The implementation of IXmlSerializable.ReadXml in XElement calls ReadElementFrom passing LoadOptions.None. Outside of DataContractSerializer usage which uses IXmlSerializable.ReadXml and therefore sets LoadOptions to None, it might be worth adding a documentation note stating there can be a significant performance difference when passing LoadOptions.SetLineInfo. If the underlying XmlReader supports line info, and LoadOptions.SetLineInfo is passed as an option, then this performance improvement gets bypassed. So not an issue for DataContractSerialzier, but might be an issue for other uses. |
XContainer's IXmlSerializable/load path materialized each text node individually via AddStringSkipNotify, which concatenates immutable strings. When an XmlReader surfaces a single logical text value as many small consecutive text nodes (e.g. DataContractSerializer reading over a stream, or IXmlSerializable.ReadXml), this degrades to O(n^2) and makes deserialization pathologically slow.
Buffer runs of adjacent text (Text/SignificantWhitespace/Whitespace) into a StringBuilder and materialize them in a single pass, keeping the load linear. Rather than sprinkling flush calls across every non-text branch, each of the four ContentReader read methods is split in two: a fast path that appends coalescable text to the buffer and returns, and a slow path that flushes the buffer once before switching over any non-text node. The buffer is also flushed at the end of each driving loop so trailing document-level text (e.g. whitespace after an XDocument's root element, where the reader stops at end-of-input rather than a matching EndElement) is preserved.
The resulting node tree is identical to before: AddStringSkipNotify still coalesces consecutive strings, flushing always happens before the current container or base URI changes, and text requiring its own base URI or line info continues to fall through to a standalone XText.
Fixes #37893