diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Cache/XPathNodeInfoAtom.cs b/src/libraries/System.Private.Xml/src/System/Xml/Cache/XPathNodeInfoAtom.cs index 298001f4db95da..c70d3897bef7f7 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Cache/XPathNodeInfoAtom.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Cache/XPathNodeInfoAtom.cs @@ -4,6 +4,7 @@ using System; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Runtime.CompilerServices; using System.Text; using System.Xml.XPath; @@ -253,23 +254,25 @@ public override int GetHashCode() { if (_hashCode == 0) { - int hashCode; - - // Start with local name - hashCode = _localNameHash; - - // Add page indexes - unchecked - { - if (_pageSibling != null) - hashCode += (hashCode << 7) ^ _pageSibling[0].PageInfo!.PageNumber; - - if (_pageParent != null) - hashCode += (hashCode << 7) ^ _pageParent[0].PageInfo!.PageNumber; - - if (_pageSimilar != null) - hashCode += (hashCode << 7) ^ _pageSimilar[0].PageInfo!.PageNumber; - } + // All fields checked by Equals must be included here to avoid hash collisions. + // Equals uses reference equality for strings (atomized via NameTable) and + // page arrays, so we use RuntimeHelpers.GetHashCode for a 1:1 match. + HashCode hc = default; + hc.Add(RuntimeHelpers.GetHashCode(_localName)); + hc.Add(RuntimeHelpers.GetHashCode(_namespaceUri)); + hc.Add(RuntimeHelpers.GetHashCode(_prefix)); + if (_baseUri is not null) + hc.Add(RuntimeHelpers.GetHashCode(_baseUri)); + if (_pageSibling is not null) + hc.Add(RuntimeHelpers.GetHashCode(_pageSibling)); + if (_pageParent is not null) + hc.Add(RuntimeHelpers.GetHashCode(_pageParent)); + if (_pageSimilar is not null) + hc.Add(RuntimeHelpers.GetHashCode(_pageSimilar)); + hc.Add(_lineNumBase); + hc.Add(_linePosBase); + + int hashCode = hc.ToHashCode(); // Save hashcode. Don't save 0, so that it won't ever be recomputed. _hashCode = ((hashCode == 0) ? 1 : hashCode); diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs index 89c814b2a27b9e..c32af941e9c937 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs @@ -167,7 +167,7 @@ private enum InitInputType private int _attrDuplWalkCount; private bool _attrNeedNamespaceLookup; private bool _fullAttrCleanup; - private NodeData[]? _attrDuplSortingArray; + private HashSet? _attrDuplSet; // name table private XmlNameTable _nameTable; @@ -287,7 +287,7 @@ private enum InitInputType private const int NodesInitialSize = 8; private const int InitialParsingStatesDepth = 2; private const int MaxByteSequenceLen = 6; // max bytes per character - private const int MaxAttrDuplWalkCount = 250; + private const int MaxAttrDuplWalkCount = 64; private const int MinWhitespaceLookahedCount = 4096; private const string XmlDeclarationBeginning = "(NodeData.AtomizedNameEqualityComparer.Instance); + _attrDuplSet.Clear(); - NodeData attr1 = _attrDuplSortingArray[0]; - for (int i = 1; i < _attrCount; i++) + for (int i = _index + 1; i < _index + 1 + _attrCount; i++) { - NodeData attr2 = _attrDuplSortingArray[i]; - if (Ref.Equal(attr1.localName, attr2.localName) && Ref.Equal(attr1.ns, attr2.ns)) + if (!_attrDuplSet.Add(_nodes[i])) { - Throw(SR.Xml_DupAttributeName, attr2.GetNameWPrefix(_nameTable), attr2.LineNo, attr2.LinePos); + Throw(SR.Xml_DupAttributeName, _nodes[i].GetNameWPrefix(_nameTable), _nodes[i].LineNo, _nodes[i].LinePos); } - attr1 = attr2; } } } diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImplHelpers.cs b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImplHelpers.cs index bedbb3ca90eb7d..633b58e50e6669 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImplHelpers.cs +++ b/src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImplHelpers.cs @@ -8,6 +8,7 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.IO; +using System.Runtime.CompilerServices; using System.Runtime.Versioning; using System.Security; using System.Text; @@ -705,6 +706,30 @@ int IComparable.CompareTo(object? obj) return 1; } } + + internal sealed class AtomizedNameEqualityComparer : IEqualityComparer + { + internal static readonly AtomizedNameEqualityComparer Instance = new AtomizedNameEqualityComparer(); + + public bool Equals(NodeData? x, NodeData? y) + { + if (x is null) + { + return y is null; + } + + return y is not null + && Ref.Equal(x.localName, y.localName) + && Ref.Equal(x.ns, y.ns); + } + + public int GetHashCode(NodeData node) + { + return HashCode.Combine( + RuntimeHelpers.GetHashCode(node.localName), + RuntimeHelpers.GetHashCode(node.ns)); + } + } } // diff --git a/src/libraries/System.Private.Xml/tests/Misc/AttributeReadingPerformanceTests.cs b/src/libraries/System.Private.Xml/tests/Misc/AttributeReadingPerformanceTests.cs new file mode 100644 index 00000000000000..2277076f0606e6 --- /dev/null +++ b/src/libraries/System.Private.Xml/tests/Misc/AttributeReadingPerformanceTests.cs @@ -0,0 +1,183 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.IO; +using System.Text; +using System.Threading; +using System.Xml.XPath; +using Xunit; + +namespace System.Xml.Tests +{ + public abstract class AttributeReadingPerformanceTests + { + // This should match the value in XmlTextReaderImpl + private const int MaxAttrDuplWalkCount = 64; + + private const int SmallN = 2_000; + private const int LargeN = 20_000; + private const double SizeRatio = (double)LargeN / SmallN; + private const double MaxRatioMultiplier = 4; + private static readonly TimeSpan s_timeout = TimeSpan.FromSeconds(60); + + protected abstract void ReadFully(string xml, CancellationToken ct); + + [Fact] + public void AttributeDuplicatesCheck_LongUris_SameLocalName_AboveThreshold() + { + // Exercises the HashSet duplicate-check path (number of attributes >= threshold) + AssertLinearScaling( + n => GenerateDoc(n, attrCount: MaxAttrDuplWalkCount, longUris: true, distinctLocalNames: false)); + } + + [Fact] + public void AttributeDuplicatesCheck_ShortUris_SameLocalName_BelowThreshold() + { + // Pairwise walk path (number of attributes < threshold) + AssertLinearScaling( + n => GenerateDoc(n, attrCount: MaxAttrDuplWalkCount - 1, longUris: false, distinctLocalNames: false)); + } + + [Fact] + public void AttributeDuplicatesCheck_LongUris_DistinctLocalNames_AboveThreshold() + { + // Distinct localNames starting with same letter (to bypass some optimizations) + AssertLinearScaling( + n => GenerateDoc(n, attrCount: MaxAttrDuplWalkCount, longUris: true, distinctLocalNames: true)); + } + + [Fact] + public void AttributeDuplicatesCheck_LongUris_SameLocalName_WellAboveThreshold() + { + // Larger amount of attributes — well above the threshold + AssertLinearScaling( + n => GenerateDoc(n, attrCount: MaxAttrDuplWalkCount * 4, longUris: true, distinctLocalNames: false)); + } + + [Fact] + public void AttributeDuplicatesCheck_ShortUris_DistinctLocalNames_BelowThreshold() + { + // Below threshold with distinct localNames + AssertLinearScaling( + n => GenerateDoc(n, attrCount: MaxAttrDuplWalkCount - 1, longUris: false, distinctLocalNames: true)); + } + + // We're doing full string.Equals on DEBUG on top of Ref.Equal which makes it quadratic. +#if !DEBUG + [Fact] + public void AttributeDuplicatesCheck_LongUris_SameLocalName_BelowThreshold() + { + // Pairwise walk path with long URIs — only valid in Release + AssertLinearScaling( + n => GenerateDoc(n, attrCount: MaxAttrDuplWalkCount - 1, longUris: true, distinctLocalNames: false)); + } +#endif + + private void AssertLinearScaling(Func generateDoc) + { + using CancellationTokenSource cts = new(s_timeout); + CancellationToken ct = cts.Token; + + ReadFully(generateDoc(SmallN), ct); + + long smallTime = MeasureRead(generateDoc(SmallN), ct); + long largeTime = MeasureRead(generateDoc(LargeN), ct); + + double maxAllowed = SizeRatio * MaxRatioMultiplier; + double actualRatio = (double)largeTime / Math.Max(smallTime, 1); + + Assert.True(actualRatio <= maxAllowed, + $"Scaling ratio {actualRatio:F1}x exceeded {maxAllowed:F1}x limit " + + $"(input grew {SizeRatio:F0}x). " + + $"Small ({SmallN}): {smallTime} ms, Large ({LargeN}): {largeTime} ms."); + } + + private long MeasureRead(string doc, CancellationToken ct) + { + Stopwatch sw = Stopwatch.StartNew(); + ReadFully(doc, ct); + return sw.ElapsedMilliseconds; + } + + private static string GenerateDoc(int n, int attrCount, bool longUris, bool distinctLocalNames) + { + int childCount = n / 2; + string garbageText = longUris ? new string('x', childCount) : "ns"; + + StringBuilder child = new(); + child.Append("= 0; i--) + { + if (distinctLocalNames) + child.Append($" x{i:X4}:a{i:X4}=\"\""); + else + child.Append($" x{i:X4}:a=\"\""); + } + child.Append("/>"); + string childElement = child.ToString(); + + StringBuilder sb = new(); + sb.Append("'); + for (int i = 0; i < childCount; i++) + { + sb.Append(childElement); + } + sb.Append(""); + + return sb.ToString(); + } + } + + // XmlReader.Create wraps XmlTextReaderImpl in XmlAsyncCheckReader + public class AttributeReadingPerformanceTests_XmlReaderCreate : AttributeReadingPerformanceTests + { + protected override void ReadFully(string xml, CancellationToken ct) + { + using XmlReader xr = XmlReader.Create(new StringReader(xml)); + while (xr.Read()) { ct.ThrowIfCancellationRequested(); } + } + } + + // XmlNodeReader reads from a pre-parsed DOM tree (XmlDocument). + // Construction is not timed — only the read traversal. + public class AttributeReadingPerformanceTests_XmlNodeReader : AttributeReadingPerformanceTests + { + protected override void ReadFully(string xml, CancellationToken ct) + { + XmlDocument doc = new(); + doc.LoadXml(xml); + using XmlReader xr = new XmlNodeReader(doc); + while (xr.Read()) { ct.ThrowIfCancellationRequested(); } + } + } + + // XPathNavigatorReader reads from an XPathDocument's XPath data model + public class AttributeReadingPerformanceTests_XPathNavigatorReader : AttributeReadingPerformanceTests + { + protected override void ReadFully(string xml, CancellationToken ct) + { + ct.ThrowIfCancellationRequested(); + XPathDocument doc = new(new StringReader(xml)); + ct.ThrowIfCancellationRequested(); + using XmlReader xr = doc.CreateNavigator().ReadSubtree(); + while (xr.Read()) { ct.ThrowIfCancellationRequested(); } + } + } + + // XmlReader.Create(XmlReader, settings) wraps a reader in XmlSubtreeReader + public class AttributeReadingPerformanceTests_WrappedReader : AttributeReadingPerformanceTests + { + protected override void ReadFully(string xml, CancellationToken ct) + { + using XmlReader inner = XmlReader.Create(new StringReader(xml)); + using XmlReader xr = XmlReader.Create(inner, new XmlReaderSettings()); + while (xr.Read()) { ct.ThrowIfCancellationRequested(); } + } + } +} diff --git a/src/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj b/src/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj index c670a2f39d5e12..b36f101e0cacad 100644 --- a/src/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj +++ b/src/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj @@ -32,6 +32,7 @@ +