Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor root-level serialization logic and polymorphic value handling. #72789

Merged
merged 8 commits into from
Jul 29, 2022

Conversation

eiriktsarpalis
Copy link
Member

Refactors the root-level serialization/deserialization routines so that

  1. Both JsonSerializerOptions and JsonTypeInfo<T> overloads in JsonSerializer call into common helper methods, ensuring that metadata is being handled consistently regardless of invoked overload.
  2. Polymorphic handling of root-level object values is moved to the converter layer. Fixes System.Text.Json instances of polymorphic types emit type discriminator when serialized as object types #72187 and System.Text.Json uses inconsistent polymorphism semantics with custom object converters #72681.

@ghost
Copy link

ghost commented Jul 25, 2022

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

Refactors the root-level serialization/deserialization routines so that

  1. Both JsonSerializerOptions and JsonTypeInfo<T> overloads in JsonSerializer call into common helper methods, ensuring that metadata is being handled consistently regardless of invoked overload.
  2. Polymorphic handling of root-level object values is moved to the converter layer. Fixes System.Text.Json instances of polymorphic types emit type discriminator when serialized as object types #72187 and System.Text.Json uses inconsistent polymorphism semantics with custom object converters #72681.
Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: -

@eiriktsarpalis
Copy link
Member Author

It must be noted that this change introduces a slight performance regression, specifically when it comes to serializing small root-level object values:

System.Text.Json.Serialization.Tests.WriteJson<Int32>.SerializeObjectProperty

Method Job Toolchain Mean Error StdDev Median Min Max Ratio MannWhitney(3%) RatioSD Gen 0 Allocated Alloc Ratio
SerializeObjectProperty Job-LSKJVH main 235.7 ns 5.44 ns 5.82 ns 236.1 ns 226.0 ns 246.8 ns 1.00 Base 0.00 0.0254 264 B 1.00
SerializeObjectProperty Job-KLBYHL PR 249.3 ns 8.82 ns 10.16 ns 249.8 ns 231.8 ns 270.1 ns 1.06 Slower 0.04 0.0260 264 B 1.00

Fundamentally, it is caused by this change removing the hardcoded polymorphic serialization for root-level object values. Instead of directly resolving the metadata for the runtime type, the metadata for object is consulted before proceeding with polymorphic serialization. As such, in the worst-case scenario, two lookups need to be made against the metadata cache instead of just one.

I've been able to recover some of the performance losses by employing a secondary LRU cache specifically used for root-level polymorphic types. I think we should merge this PR and accept the performance regression, as it is essential for getting #72187 and #72681 fixed. The performance regression only impacts small, root-level polymorphic values and does not affect the statically typed APIs or the source generator which are more performance-minded.

cc @jeffhandley @steveharter @ericstj

@krwq
Copy link
Member

krwq commented Jul 29, 2022

If incorrect code or code wrote under wrong assumptions is faster then there is no perf regression to me. We should baseline from correct code. If there are any quick wins to gain the perf back then even better

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Jul 29, 2022

I made a few tweaks to the caching strategy in 97a8b7a, caching the resolved JsonTypeInfo<object> instead of using a secondary LRU cache for polymorphic types, which has contributed to even more performance gains:

Method Job Toolchain Mean Error StdDev Median Min Max Ratio MannWhitney(3%) RatioSD Gen 0 Allocated Alloc Ratio
SerializeObjectProperty Job-LZOHNV main 257.1 ns 10.66 ns 11.85 ns 259.2 ns 238.0 ns 281.6 ns 1.00 Base 0.00 0.0261 264 B 1.00
SerializeObjectProperty Job-RKZQDC PR 268.1 ns 7.07 ns 7.85 ns 268.3 ns 247.5 ns 279.9 ns 1.05 Same 0.06 0.0258 264 B 1.00

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.Json instances of polymorphic types emit type discriminator when serialized as object types
2 participants