[release/7.0] Fix delegate allocation in CachingContext.GetOrAddJsonTypeInfo

[eiriktsarpalis]

Fixes #80430.

Customer Impact

Fixes a customer reported performance regression in STJ's metadata caching implementation, which will a allocate a new delegate on every metadata lookup operation. This becomes particularly noticeable when serializing object graphs containing many polymorphic values, for example large non-generic collections.

Testing

N/A.

Risk

Low. It is a small change that caches the metadata factory delegate in a field.

[ghost]

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

Issue Details

---

Fixes #80430.

Customer Impact

Fixes a customer reported performance regression in STJ's metadata caching implementation, which will a allocate a new delegate on every metadata lookup operation. This becomes particularly noticeable when serializing object graphs containing many polymorphic values, for example large non-generic collections.

Testing

N/A.

Risk

Low. It is a small change that caches the metadata factory delegate in a field.

Author:	eiriktsarpalis
Assignees:	eiriktsarpalis
Labels:	area-System.Text.Json
Milestone:	7.0.x

[ericstj]

Needs the servicing authoring. cc @carlossanlop

[ericstj]

Test failing is JsonSerializerOptions_ReuseConverterCaches -- seems significant.

Child exception:
  Xunit.Sdk.NotNullException: Assert.NotNull() Failure
   at System.Text.Json.Serialization.Tests.CacheTests.<JsonSerializerOptions_ReuseConverterCaches>g__CreateCacheOptionsAccessor|9_1() in /_/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs:line 305
   at System.Text.Json.Serialization.Tests.CacheTests.<>c.<JsonSerializerOptions_ReuseConverterCaches>b__9_0() in /_/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs:line 271
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr) in /_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs:line 64

Child process:
  System.Text.Json.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.Text.Json.Serialization.Tests.CacheTests+<>c Void <JsonSerializerOptions_ReuseConverterCaches>b__9_0()

[eiriktsarpalis]

Approved by Tactics over email.

[eiriktsarpalis]

Test failing is JsonSerializerOptions_ReuseConverterCaches -- seems significant.

It's a reflection-based test validating that the CachingContext.Options property property exists:

https://github.com/dotnet/runtime/blob/release/7.0/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs#L304-L305

I think it's because the CachingContext.Options ends up being trimmed since the change makes it dead code. If that's the case, I'm surprised that this isn't failing in main.

[ericstj]

LGTM

[Sergio0694]

Just curious, will this also be shipped for the OOB package? 🙂

[carlossanlop]

Just curious, will this also be shipped for the OOB package? 🙂

Yep. This commit will take care of that: 81a7358

[Sergio0694]

Sweet, that's awesome! Thank you 😄

[carlossanlop]

Approved by Tactics (7.0.3).
Signed off by area owners.
Required OOB changes look correct (and may be reused by the other Json PR if it also gets merged: #80576).
CI failure was an unrelated timeout cancellation (Linq). No test-specific failures logged.
Ready to merge.