Fix JsonSerializerOptions.GetConverter recreating built-in converters #65799
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json Issue DetailsFix #65770. @layomia do you think is this the right approach? I changed @ericstj Do you think this meets the bar for 6.0 servicing? I can imagine certain use cases where this could incur a substantial amount of added allocations (and it's a regression from 5.0)
|
| @@ -99,7 +99,6 @@ private void EnsurePushCapacity() | |||
| public JsonConverter Initialize(Type type, JsonSerializerOptions options, bool supportContinuation) | |||
| { | |||
| JsonTypeInfo jsonTypeInfo = options.GetOrAddJsonTypeInfoForRootType(type); | |||
| Debug.Assert(options == jsonTypeInfo.Options); | |||
There was a problem hiding this comment.
This assertion has been invalidated by #64646, however it wasn't being caught by our test suite until now.
There was a problem hiding this comment.
I assume it would still be true if the new comparer introduced in #64646 were used.
There was a problem hiding this comment.
== still checks for reference equality, the comparer is internal implementation detail when initializing the caches.
| @@ -99,7 +99,6 @@ private void EnsurePushCapacity() | |||
| public JsonConverter Initialize(Type type, JsonSerializerOptions options, bool supportContinuation) | |||
| { | |||
| JsonTypeInfo jsonTypeInfo = options.GetOrAddJsonTypeInfoForRootType(type); | |||
| Debug.Assert(options == jsonTypeInfo.Options); | |||
There was a problem hiding this comment.
I assume it would still be true if the new comparer introduced in #64646 were used.
| if (!IsInitializedForReflectionSerializer) | ||
| { | ||
| InitializeForReflectionSerializer(); | ||
| } |
There was a problem hiding this comment.
I don't think calling this method should root the reflection-based metadata gathering code paths, potentially masking future bugs with source-gen.
There was a problem hiding this comment.
Thanks. What is the distinction between rooting the reflection-based metadata gathering paths and rooting the built-in converter factories that depend on said reflection-based metadata gathering paths? Under the current setup an options instance not configured for reflection will eagerly return converters that do require reflection.
There was a problem hiding this comment.
Based on your remarks on this PR #51897 I'm guessing that this is only meant as an entrypoint for reflection-based serialization anyways.
There was a problem hiding this comment.
Under the current setup an options instance not configured for reflection will eagerly return converters that do require reflection.
Hmm what do you mean? The source generator doesn't use this method GetConverter(Type), so we left the reflection to avoid breaking folks that used it pre .NET 6.0.
| RunTest<Point_2D>(); | ||
|
|
||
| void RunTest<TConverterReturn>() | ||
| RemoteExecutor.Invoke(static () => |
There was a problem hiding this comment.
this feels a bit like a unit test rather than functional test because it tests an implementation detail. I'll suggest to move it to unit tests.
eiriktsarpalis
deleted the
getconverter-builtinconverter-reallocation
branch
ghost
locked as resolved and limited conversation to collaborators
Fix #65770.
@layomia do you think is this the right approach? I changed
GetConverterso that it now callsInitializeForReflectionSerializerinstead ofRootBuiltInConvertersince it already has a flag guarding access. The only difference is that additionally roots the reflection-basedJsonTypeInfofactory delegate. I suspect this is fine sinceGetConverteris already markedRequiresUnreferencedCodeand might actually fix a broken corner case (see changes in test code).@ericstj Do you think this meets the bar for 6.0 servicing? I can imagine certain use cases where this could incur a substantial amount of added allocations (and it's a regression from 5.0)