[XmlSerializer] Avoid IList/Array.GetValue path for array item serialization in reflection writer

[Copilot]

XmlSerializer’s reflection path was serializing arrays through IList, which can devolve to Array.GetValue per element and add avoidable overhead in tight loops. This change introduces an explicit array path in ReflectionXmlSerializationWriter.WriteArrayItems to keep array iteration on the direct array/enumerator path.

• Problem

  • WriteArrayItems treated arrays as IList, so element access used arr[i] and could hit slower Array.GetValue behavior for primary-type arrays.

• Change

  • Added an early if (o is Array array) branch in ReflectionXmlSerializationWriter.WriteArrayItems.
  • Iterates array items directly and forwards each item to WriteElements.
  • Hoisted choiceSources cast to a single Array? choiceArray and reused it across array/list/enumerable paths.

• Behavioral scope

  • Serialization output semantics are unchanged.
  • Change is localized to iteration strategy in the reflection serializer array-item loop.

Array? choiceArray = choiceSources as Array;

if (o is Array array)
{
    IEnumerator e = array.GetEnumerator();
    int c = 0;
    while (e.MoveNext())
    {
        object ai = e.Current;
        object? choiceSource = choiceArray?.GetValue(c++);
        WriteElements(ai, choiceSource, elements, text, choice, true, true);
    }

    return;
}

Fixes #67221

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• bla
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing (dns block)
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing urity.Cryptograp-DURTBLDENV_FRIENDLY=Debug c.c.o.d -o CMake/usr/bin/sh _ALIGNED SSUME_ALIGNED TARGET_UNIX -DUR-D_TIME_BITS=64 (dns block)
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing k/_temp/ghcca-node/node/bin/bash -DFALLBACK_OS_Igit d/lib -DDEBUG Native_EXPORTS -DTARGET_64BIT -D/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-U0 (dns block)
• foo
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing (dns block)
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing urity.Cryptograp-DURTBLDENV_FRIENDLY=Debug c.c.o.d -o CMake/usr/bin/sh _ALIGNED SSUME_ALIGNED TARGET_UNIX -DUR-D_TIME_BITS=64 (dns block)
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing k/_temp/ghcca-node/node/bin/bash -DFALLBACK_OS_Igit d/lib -DDEBUG Native_EXPORTS -DTARGET_64BIT -D/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-U0 (dns block)
• notfound.invalid.corp.microsoft.com
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing (dns block)
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing urity.Cryptograp-DURTBLDENV_FRIENDLY=Debug c.c.o.d -o CMake/usr/bin/sh _ALIGNED SSUME_ALIGNED TARGET_UNIX -DUR-D_TIME_BITS=64 (dns block)
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing k/_temp/ghcca-node/node/bin/bash -DFALLBACK_OS_Igit d/lib -DDEBUG Native_EXPORTS -DTARGET_64BIT -D/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-U0 (dns block)
• test.test
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing (dns block)
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing urity.Cryptograp-DURTBLDENV_FRIENDLY=Debug c.c.o.d -o CMake/usr/bin/sh _ALIGNED SSUME_ALIGNED TARGET_UNIX -DUR-D_TIME_BITS=64 (dns block)
  • Triggering command: /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet /home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-linux-Debug-x64/dotnet exec --runtimeconfig System.Private.Xml.Tests.runtimeconfig.json --depsfile System.Private.Xml.Tests.deps.json /home/REDACTED/.nuget/packages/microsoft.dotnet.xunitconsoleREDACTED/2.9.3-beta.26257.113/build/../tools/net/xunit.console.dll System.Private.Xml.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing k/_temp/ghcca-node/node/bin/bash -DFALLBACK_OS_Igit d/lib -DDEBUG Native_EXPORTS -DTARGET_64BIT -D/home/REDACTED/work/runtime/runtime/artifacts/bin/testhost/net11.0-U0 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[StephenMolloy]

[Copilot]

Pull request overview

This PR updates the reflection-based XmlSerializer writer’s array-item serialization loop to treat Array inputs explicitly rather than flowing through the IList path, reducing per-element overhead associated with IList indexing for arrays.

Changes:

• Adds an o is Array fast-path in ReflectionXmlSerializationWriter.WriteArrayItems that iterates via Array.GetEnumerator() and forwards items to WriteElements.
• Hoists choiceSources as Array into a single Array? choiceArray and reuses it across array/list/enumerable paths.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "66cbf868c49a015be2cef62009c1955cd348a92c",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "3a1a86525ba8649767249acefa9cad7abeaa4fe4",
  "last_reviewed_commit": "66cbf868c49a015be2cef62009c1955cd348a92c",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "3a1a86525ba8649767249acefa9cad7abeaa4fe4",
  "last_recorded_worker_run_id": "29680699499",
  "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": "66cbf868c49a015be2cef62009c1955cd348a92c",
      "review_id": 4730564956
    }
  ]
}

[github-actions]

Holistic Review

Motivation: WriteArrayItems in the reflection-based XmlSerializer writer treated arrays through the generic IList path, where element access (arr[i]) on a boxed primitive array can devolve into slower Array.GetValue behavior. The PR (fixing #67221) aims to keep array serialization on the direct SZ-array enumerator path to reduce per-element overhead in tight serialization loops.

Approach: An explicit if (o is Array array) branch is added ahead of the IList branch, iterating via array.GetEnumerator() (the optimized single-dimension array enumerator) and forwarding each element to WriteElements. The choiceSources cast is hoisted once into a local Array? choiceArray and reused across the array, list, and enumerable branches, removing the repeated per-iteration (Array?)choiceSources cast. The list branch replaces o as IList + null-check with an o is IList list pattern.

Summary: The change is small, well-scoped, and behavior-preserving. Ordering and choice-source indexing are unchanged: the array branch increments c in lockstep with element iteration exactly as the pre-existing enumerable branch does, so choiceArray?.GetValue(c++) yields the same values as the previous ((Array?)choiceSources)?.GetValue(i). Because arrays already implement IList, they would previously have taken the list path; routing them through the array branch first is functionally equivalent for single-dimension arrays (the only kind produced here). Multidimensional arrays are not a concern in this code path, and the fallback enumerable branch and list branch remain intact. No test changes accompany the PR, which is acceptable given existing System.Private.Xml serialization coverage exercises array members and output semantics are unchanged; CI results should confirm no regressions. The only nit is a typo in the new comment ("is know" → "is known"), noted inline. 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 · 41.4 AIC · ⌖ 9.6 AIC · ⊞ 10K

[mangod9]

Doing a copilot review shows that the optimization doesnt actually work. Can you please check if this analysis holds -- feels like it based on the source pointers below?

The array special-case doesn''t actually avoid Array.GetValue — both paths call the same InternalGetValue, so this adds an allocation and a MoveNext per item for no benefit.

The premise is that IList indexing (arr[i]) hits a slow Array.GetValue path while Array.GetEnumerator() avoids it. But both bottom out in the identical internal method:

• IList path: arr[i] → Array''s explicit IList.this[int].get → GetValue(index) → InternalGetValue.
  

  runtime/src/libraries/System.Private.CoreLib/src/System/Array.cs

  Lines 2129 to 2133 in 13cd8e3

  	//
  	// Special case for 0 length List
  	// accept -1 and 0 as valid startIndex for compablility reason.
  	//
  	if (startIndex != -1 && startIndex != 0)

• Enumerator path: Array.GetEnumerator() always returns a non-generic ArrayEnumerator (no SZ fast-path here)…
  https://github.com/dotnet/runtime/blob/13cd8e344c41d55e63b2be288b1b8a1e2596b32d/src/libraries/System.Private.CoreLib/src/System/Array.cs#L5985-L5989
  …whose Current calls the same InternalGetValue:
  

  runtime/src/libraries/System.Private.CoreLib/src/System/Array.Enumerators.cs

  Lines 63 to 95 in 13cd8e3

  	public void Reset()
  	{
  	_index = -1;
  	}
  	}
  	internal abstract class SZGenericArrayEnumeratorBase : IDisposable
  	{
  	protected int _index;
  	protected readonly int _endIndex;
  	protected SZGenericArrayEnumeratorBase(int endIndex)
  	{
  	_index = -1;
  	_endIndex = endIndex;
  	}
  	[MethodImpl(MethodImplOptions.AggressiveInlining)]
  	public bool MoveNext()
  	{
  	int index = _index + 1;
  	if ((uint)index < (uint)_endIndex)
  	{
  	_index = index;
  	return true;
  	}
  	_index = _endIndex;
  	return false;
  	}
  	public void Reset() => _index = -1;

So per element both do bounds-check + InternalGetValue + box. The enumerator variant is actually slightly worse: it heap-allocates the ArrayEnumerator (the indexer path allocates nothing) and adds a MoveNext() call on top of the identical Current work.

The boxing is the real cost, and it''s unavoidable with this shape: WriteElements(object? o, …) takes each item as object, so value-type elements are boxed no matter how we iterate. Changing the iteration strategy alone can''t help. Actually eliminating the boxing (to address #67221) would require generic specialization of the write loop by T — matching what the IL-generated writer does with typed ldelem indexing in XmlSerializationWriterILGen.WriteArrayItems, where arrays go through an indexed for loop and the enumerator is reserved for IsEnumerable collections — the opposite of the assumption here.

Suggest dropping the is Array branch (keeping only the choiceArray cast-hoist cleanup, if desired).

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[mconnew]

The latest iteration of this change seems to have dropped the check if it's an array and then use array.GetEnumerator(). This appears to be in response to the feedback from @mangod9 here. This makes the PR description wrong. I do have a suggestion for a better approach. Similar to how you have the generic method TryWritePrimitiveItems<T> for use with a known set of primitive element types and you call it passing the generic type, you can do the same thing for all types. You can get the MethodInfo for an open generic method and instantiate an instance for a specific type. The method signature itself can have zero references to the generic type in any parameters or the return type so you can have a single delegate signature definition so you can call it easily without using Invoke. Then you have a cache of instances stored in a Dictionary or HashTable (or even store it something like an ElementAccessor instance). This way you can do direct array references with zero overhead. The overhead would be the first time you instantiate the closed generic instance of a delegate. If you would like me to provide some more concrete code on how this would look if my description is too vague, let me know.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationWriter.cs:166

• This method still treats arrays as IList (IList? list = o as IList;), so for non-primitive arrays the per-element access can still devolve to Array.GetValue via the IList indexer. The PR description mentions adding an explicit o is Array branch to avoid that path, but the current implementation only adds a primitive fast-path. Either implement the array-specific iteration branch as described (to avoid IList indexing for arrays generally), or update the PR description to match the actual scope (primitive-only optimization).

            bool isListText = text != null && text.IsList && elements.Length == 0;

            IList? list = o as IList;
            if (list is not null && list.Count == 0)
            {
                return;
            }

            if (TryWritePrimitiveItems(elements, text, choice, o))

src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationWriter.cs:209

• In the enumerable (non-IList) path, c is incremented when fetching choiceSource (GetValue(c++)) before the isListText whitespace check. This causes a leading space to be written for the first item when isListText is true (since c is already 1). The spacing check needs to use the pre-increment index (or increment after writing).

                    int c = 0;
                    while (e.MoveNext())
                    {
                        object ai = e.Current;
                        object? choiceSource = choiceArray?.GetValue(c++);

                        if (isListText && c > 0)
                        {
                            WriteValue(" ");
                        }

[StephenMolloy]

I do have a suggestion for a better approach. Similar to how you have the generic method TryWritePrimitiveItems<T> for use with a known set of primitive element types and you call it passing the generic type, you can do the same thing for all types. You can get the MethodInfo for an open generic method and instantiate an instance for a specific type. The method signature itself can have zero references to the generic type in any parameters or the return type so you can have a single delegate signature definition so you can call it easily without using Invoke. Then you have a cache of instances stored in a Dictionary or HashTable (or even store it something like an ElementAccessor instance). This way you can do direct array references with zero overhead. The overhead would be the first time you instantiate the closed generic instance of a delegate.

So, I'm not sure this is really an improvement, or even feasible? The reflection-based serializer has to support both NativeAOT and NO DynamicCode scenarios. The latter means we still need a non-MakeGenericMethod path, and the former means we would have to have a version of the code that statically roots the bound generics anyway. The big "if" block is ugly, but it only runs once per collection. We could cache the result of this if block on the TypeDesc, but the cost of storing and doing that lookup properly does not look like a huge savings. We can add a check for IsValueType before the "if" block though, so reference types don't have to fall through all the 'if's.

[azure-pipelines]

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.

[StephenMolloy]

/ba-g helix failures unrelated