Improve perf of attribute duplicate check

[krwq]

No description provided.

[azure-pipelines]

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.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR updates System.Private.Xml to reduce the cost of attribute duplicate-name checking during XML parsing, and adds tests intended to catch non-linear scaling regressions in attribute-heavy documents.

Changes:

• Switch XmlTextReaderImpl’s “many attributes” duplicate-check path from sort+walk to a HashSet<NodeData> with an atomized-name comparer; adjust the related threshold constant.
• Add AtomizedNameEqualityComparer for NodeData to support reference-based hashing/equality of atomized names.
• Add timing-based scaling tests for attribute reading across several reader entry points; update XPathNodeInfoAtom.GetHashCode() to better align with its Equals implementation.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImpl.cs	Reworks attribute duplicate checking to use a reusable HashSet and changes the walk threshold constant.
src/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextReaderImplHelpers.cs	Adds an atomized-name equality comparer for NodeData to support the new HashSet logic.
src/libraries/System.Private.Xml/src/System/Xml/Cache/XPathNodeInfoAtom.cs	Updates cached hash computation to include all fields compared by Equals.
src/libraries/System.Private.Xml/tests/Misc/AttributeReadingPerformanceTests.cs	Introduces performance-scaling tests for attribute reading across multiple reader types.
src/libraries/System.Private.Xml/tests/System.Private.Xml.Tests.csproj	Adds the new performance test file to the test project compilation list.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "e2d13bda2cd7f5b105dd23ea2caa1a57d5b06a50",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "402ed14c4080491d0965638b7a1dfd673239b586",
  "last_reviewed_commit": "e2d13bda2cd7f5b105dd23ea2caa1a57d5b06a50",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "402ed14c4080491d0965638b7a1dfd673239b586",
  "last_recorded_worker_run_id": "29686419229",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "e2d13bda2cd7f5b105dd23ea2caa1a57d5b06a50",
      "review_id": 4730740190
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Duplicate-attribute detection in XmlTextReaderImpl previously fell back to an Array.Sort + adjacent-walk once an element accumulated many first-character hash collisions. For elements with a large number of attributes this sort-based path is O(n log n) with allocation churn, and the walk threshold (250) allowed a long O(n^2) pre-check window, producing poor scaling on attribute-heavy documents. The PR reworks the fallback to an atomized-name HashSet lookup and lowers the threshold to bound the quadratic window.

Approach: The many-attribute branch of AttributeDuplCheck now uses a reusable HashSet<NodeData> keyed by a new NodeData.AtomizedNameEqualityComparer that hashes and compares on the atomized (reference-equal) localName/ns pair, matching the semantics of the prior Ref.Equal comparison. The set is allocated lazily and Clear()ed per element for reuse. MaxAttrDuplWalkCount drops from 250 to 64 to shorten the O(n^2) pre-walk. A tangential change aligns XPathNodeInfoAtom.GetHashCode() with its Equals by hashing every compared field via RuntimeHelpers.GetHashCode. New scaling tests parse small vs. large attribute-heavy documents across five reader entry points and assert near-linear time growth.

Summary: The core change is correct and a clear improvement. The HashSet comparer preserves the exact duplicate semantics of the removed sort+walk (reference equality on atomized localName and ns), and the reuse/Clear pattern avoids per-element allocation. The inner if (_attrCount < MaxAttrDuplWalkCount) pairwise branch remains reachable via the forced-threshold path in AddAttribute (where a real duplicate sets _attrDuplWalkCount = MaxAttrDuplWalkCount while _attrCount may still be small), so both branches are live and correct. The XPathNodeInfoAtom.GetHashCode update is consistent with Equals and safe. Remaining concerns are non-blocking and were already raised on the PR: (1) the wall-clock/ratio-based scaling tests run in the inner-loop libraries suite (which disables per-assembly parallelization) and are inherently susceptible to CI variance -- [OuterLoop] or the perf infrastructure would be more robust, though the author has chosen to gather CI signal first; (2) the duplicate MaxAttrDuplWalkCount constant is now mirrored as a literal 64 in the test, a maintenance coupling worth a comment link; (3) the not-pre-sized HashSet may resize on the first large element, a minor cost the author intentionally deferred for servicing-branch parity. None of these affect correctness. Verdict: LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 144.7 AIC · ⌖ 10.6 AIC · ⊞ 10K

[krwq]

/ba-g failures seem infra related