Optimize method invocations in System.Text.Json et al. by teo-tsirpanis · Pull Request #131018 · dotnet/runtime

teo-tsirpanis · 2026-07-18T19:45:26Z

This PR updates System.Text.Json's reflection member accesor, to use use the MethodInvoker and ConstructorInvoker APIs on supported frameworks. This is faster, and does not wrap exceptions in TargetInvocationException.

I also replaced one use of checking whether an attribute exists with IsDefined.

…structor<T>`.

… reflection member accessor.

azure-pipelines · 2026-07-18T19:46:02Z

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.

dotnet-policy-service · 2026-07-18T19:46:32Z

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

Copilot

Pull request overview

This PR updates System.Text.Json’s reflection-based MemberAccessor implementation to use MethodInvoker/ConstructorInvoker when targeting NET, and adjusts the internal parameterized-constructor delegate signature to Func<object?[], T>. It also switches a JsonConstructorAttribute presence check to IsDefined to avoid allocating the attribute instance.

Changes:

Use ConstructorInvoker/MethodInvoker in ReflectionMemberAccessor (#if NET) for faster invocation paths.
Update MemberAccessor.CreateParameterizedConstructor<T> and related emit/caching implementations to return Func<object?[], T>.
Replace GetCustomAttribute<JsonConstructorAttribute>() != null with IsDefined(...).

Reviewed changes

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

Show a summary per file

File	Description
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs	Switches reflection invocations to invokers on `NET`; adjusts constructor/property/add delegates.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitMemberAccessor.cs	Updates parameterized-constructor delegate signature to `Func<object?[], T>`.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionEmitCachingMemberAccessor.cs	Updates cached parameterized-constructor delegate signature to `Func<object?[], T>`.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/MemberAccessor.cs	Changes abstract API to `Func<object?[], T>` for parameterized constructors.
src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs	Uses `IsDefined` for `JsonConstructorAttribute` presence check.

teo-tsirpanis · 2026-07-19T01:40:23Z

            int parameterCount = constructor.GetParameters().Length;

+#if NET
+            ConstructorInvoker invoker = ConstructorInvoker.Create(constructor);
+            return arguments => (T)invoker.Invoke(arguments.AsSpan(0, parameterCount));
+#else


I'm not sure I understand; does regular Invoke skip out parameters?

Copilot

Pull request overview

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

azure-pipelines · 2026-07-19T01:39:36Z

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.

Copilot

Pull request overview

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

… getters.

MichalPetryka · 2026-07-19T02:01:07Z

+#if NET
+            MethodInvoker invoker = MethodInvoker.Create(addMethod);
+            return (collection, element) => invoker.Invoke(collection, element);
+#else
+            return (collection, element) => addMethod.InvokeNoWrapExceptions(collection, new object[] { element });
+#endif


This should be changed too.

We can't here, because both the declaring type and the parameter type must be generic. ~~Without it, we would need to do say typeof(Action<,>).MakeGenericType(collectionType, elementType) after making sure that both are reference types (because otherwise it's not AOT compatible).~~ We would also need to add another wrapping delegate to cast the object parameter to its actual type, but we don't statically know it, and we can't use reflection emit in this class.

MichalPetryka · 2026-07-19T02:01:17Z

+#if NET
+            MethodInvoker invoker = MethodInvoker.Create(getMethodInfo);
+            return obj => (TProperty)invoker.Invoke(obj)!;
+#else
+            return obj => (TProperty)getMethodInfo.InvokeNoWrapExceptions(obj, null)!;
+#endif


MichalPetryka · 2026-07-19T02:01:23Z

+#if NET
+            MethodInvoker invoker = MethodInvoker.Create(setMethodInfo);
+            return (obj, value) => invoker.Invoke(obj, value);
+#else
+            return (obj, value) => setMethodInfo.InvokeNoWrapExceptions(obj, new object[] { value });
+#endif


Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs:56

CreateParameterizedConstructor currently slices/copies parameterCount elements from the pooled arguments array and passes them directly to ConstructorInvoker/ConstructorInfo.Invoke. However, System.Text.Json deliberately skips out parameters when building the ctor-argument array (see DefaultJsonTypeInfoResolver.Helpers.cs:672-714), and the Reflection.Emit accessor compensates via an explicit ctor-param-index -> args-index mapping (ReflectionEmitMemberAccessor.cs:94-113). On platforms that use ReflectionMemberAccessor (e.g., !IsDynamicCodeCompiled), parameterCount includes out parameters, so this will read uninitialized pooled elements (or even go out of range) and then fail in invoker argument checking / parameter-count validation.

Please expand the packed non-out argument array into a full-length argument list (inserting null placeholders for out params) before invoking when any out parameters are present, keeping the existing no-out fast path.

            int parameterCount = constructor.GetParameters().Length;

#if NET
            ConstructorInvoker invoker = ConstructorInvoker.Create(constructor);
            return arguments => (T)invoker.Invoke(arguments.AsSpan(0, parameterCount));

github-actions · 2026-07-19T13:44:29Z

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "e94afd467676736c5cd4bdd64d1ce2fcf6d2b23f",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
  "last_reviewed_commit": "e94afd467676736c5cd4bdd64d1ce2fcf6d2b23f",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
  "last_recorded_worker_run_id": "29689457976",
  "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": "e94afd467676736c5cd4bdd64d1ce2fcf6d2b23f",
      "review_id": 4730875547
    }
  ]
}

github-actions

Holistic Review

Motivation: The PR is well justified. System.Text.Json's reflection-based member accessor previously routed every constructor/method/property access through MethodInfo.Invoke/ConstructorInfo.InvokeNoWrapExceptions, which allocate argument arrays and are slower than the modern MethodInvoker/ConstructorInvoker APIs. Using these on supported frameworks is a real, measurable improvement.

Approach: Sound and idiomatic. Fast paths are guarded with #if NET and fall back to the existing InvokeNoWrapExceptions path on netstandard2.0/netfx, preserving the important "no TargetInvocationException wrapping" semantics (both ConstructorInvoker.Invoke and MethodInvoker.Invoke propagate the original exception, matching InvokeNoWrapExceptions). The value-type property getter now binds an open-instance ref-receiver delegate instead of boxing through Invoke, and the IsDefined swap avoids materializing an attribute instance. Scope is tight and internal-only — no public API surface is added.

Summary: ✅ LGTM. The refactor preserves observable behavior (exception propagation, parameter-count handling, null argument passing) while reducing allocations and invocation overhead. I verified the only caller of the two-generic CreatePropertyGetter<TDeclaringType, TProperty> and the #if NET framework floor; no correctness concerns found. See the minor observation below.

Detailed Findings

✅ Exception semantics preserved

The legacy path used InvokeNoWrapExceptions specifically so exceptions (including the eight-element tuple ArgumentException) surface unwrapped. ConstructorInvoker.Invoke/MethodInvoker.Invoke also do not wrap in TargetInvocationException, so behavior is preserved on the #if NET path. Good.

✅ Parameterized constructor slicing is correct

In CreateParameterizedConstructor<T>, the input array is rented from a shared pool and may be larger than the parameter count. arguments.AsSpan(0, parameterCount) correctly bounds the span to exactly the constructor's parameters, matching the old explicit copy loop. The goto case fall-through rewrite in the 4-arg overload's #else branch is equivalent to the original per-index assignment.

✅ Value-type property getter binding

CreatePropertyGetter<TDeclaringType, TProperty> now binds an open-instance delegate. For value types it uses a ref TDeclaringType receiver delegate (ValueTypePropertyGetter) and wraps it, which is the correct way to bind an instance getter on a struct without boxing; for reference types it binds Func<TDeclaringType, TProperty> directly. This is the standard runtime pattern and matches the emit-based accessor's intent.

💡 Latent constraint on the two-generic getter (non-blocking)

Switching to CreateDelegate makes CreatePropertyGetter<TDeclaringType, TProperty> stricter than the old Invoke+cast: Delegate.CreateDelegate does not permit value-type-to-object boxing return conversions. This is safe today because the sole caller (DefaultJsonTypeInfoResolver.Union.CreatePropertyGetter<TUnion, object?>) is gated on PropertyType == typeof(object), so no boxing conversion is ever required. Worth keeping in mind if a future caller passes a TProperty that differs from the property's actual return type — the emit accessor would handle it via IL conversion, but this reflection accessor would throw at delegate creation.

✅ Test coverage

No tests are added, which is reasonable for a behavior-preserving internal refactor: the reflection member accessor is exercised extensively by existing (non-source-gen) serialization/deserialization tests across both #if NET and downlevel TFMs. CI results should confirm no regressions.

Note

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

Generated by Holistic Review · 137 AIC · ⌖ 14.9 AIC · ⊞ 10K

eiriktsarpalis

Thanks

teo-tsirpanis added 3 commits July 18, 2026 03:36

Fix nullability annotations of `MemberAccessor.CreateParameterizedCon…

017b2f5

…structor<T>`.

Use constructor and method invokers in factory methods created by the…

c689d6c

… reflection member accessor.

Use IsDefined to check for attribute's existence in a type.

0a7da8f

Copilot AI review requested due to automatic review settings July 18, 2026 19:45

github-actions Bot added the area-System.Text.Json label Jul 18, 2026

dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jul 18, 2026

Copilot started reviewing on behalf of teo-tsirpanis July 18, 2026 19:46 View session

Copilot AI reviewed Jul 18, 2026

View reviewed changes

MichalPetryka reviewed Jul 18, 2026

View reviewed changes

Comment thread ...ies/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionMemberAccessor.cs Outdated

Create property getter with CreateDelegate if possible.

8ddfc43

Copilot AI review requested due to automatic review settings July 19, 2026 01:27

Copilot started reviewing on behalf of teo-tsirpanis July 19, 2026 01:28 View session

jkotas reviewed Jul 19, 2026

View reviewed changes

Comment thread src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs Outdated

Copilot AI reviewed Jul 19, 2026

View reviewed changes

Use standard reflection polyfills.

bfa8173

Copilot AI review requested due to automatic review settings July 19, 2026 01:39

teo-tsirpanis marked this pull request as ready for review July 19, 2026 01:39

Copilot started reviewing on behalf of teo-tsirpanis July 19, 2026 01:39 View session

Copilot AI reviewed Jul 19, 2026

View reviewed changes

Use delegate that takes instance by reference, in value type property…

e94afd4

… getters.

Copilot AI review requested due to automatic review settings July 19, 2026 01:53

Copilot started reviewing on behalf of teo-tsirpanis July 19, 2026 01:53 View session

MichalPetryka suggested changes Jul 19, 2026

View reviewed changes

Copilot AI reviewed Jul 19, 2026

View reviewed changes

build-analysis Bot mentioned this pull request Jul 19, 2026

[ci-scan] Test failure: System.Security.Cryptography.X509Certificates.Tests.RevocationTests.AiaTests.AiaCompletionHasLimits #130947

Open

github-actions Bot reviewed Jul 19, 2026

View reviewed changes

eiriktsarpalis approved these changes Jul 20, 2026

View reviewed changes

Uh oh!

Conversation

teo-tsirpanis commented Jul 18, 2026

Uh oh!

azure-pipelines Bot commented Jul 18, 2026

Uh oh!

dotnet-policy-service Bot commented Jul 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

teo-tsirpanis Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

azure-pipelines Bot commented Jul 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

MichalPetryka Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

teo-tsirpanis Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

MichalPetryka Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

MichalPetryka Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Holistic Review

Detailed Findings

✅ Exception semantics preserved

✅ Parameterized constructor slicing is correct

✅ Value-type property getter binding

💡 Latent constraint on the two-generic getter (non-blocking)

✅ Test coverage

Uh oh!

eiriktsarpalis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions Bot commented Jul 19, 2026 •

edited

Loading