Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@layomia
Copy link
Contributor

@layomia layomia commented Oct 22, 2019

Ports #41482 to 3.1:

Description

This PR improves performance collections previously deserialized with the use of enumerable/dictionary converters (not to be confused with the JsonConverter feature). This includes types derived from native (System.Collections[.Generic]) collections, and native collections that don't implement IList, IDictionary, or IDictionary<>.

Converter usage is now removed for collections, excluding non-immutable types and arrays. This usage involved allocating a temporary list or dictionary to store elements of a given collection when deserializing, then passing the temp list to a converter tasked with creating and populating a return type. Converters could be removed because these types could be populated either with manual casts to ILIst, IDictionary, or IDictionary<> (IList.Add or dictionary indexer), or with a delegate to an add method retrieved and bound with reflection.

We get reduced allocations with the elimination of the temp buffers, and faster execution with simplified runtime information detection (including removing logic concerning the assignment of converters to types). See #41482 (comment) for detailed perf improvements.

See the description of #41482 for the rules governing the deserialization of collections.

Customer Impact

Significant perf improvements (ranging from 3-29% faster, up to 34.18% decrease in allocations) for custom derived collections.
Fixes https://github.com/dotnet/corefx/issues/41427 - higher order inheritance for derived types is now supported
Fixes https://github.com/dotnet/corefx/issues/40479 - reflection is now used to support more types, including non-generic Stack and Queue.
Fixes https://github.com/dotnet/corefx/issues/41034 - types that implement IDictionary are now supported for serialization, examples are Hashtable, and SortedList.

Regression?

Performance

There are no significant perf regressions accompanying the perf gains detailed in #41482 (comment).

Deserialization support

This PR is concerned with deserialization of collections. There are no functional changes to how non-collection types work, so there is little chance of rescinding support for these types.

There's a chance that this PR rescinds deserialization support for unknown collections (e.g. user types). However, I feel confident enough for this to go into preview because:

  • The core logic used by the serializer to obtain runtime information about a given collection was scoped to support types in System.Collections[.Generic], and types that implement or inherit them (for concrete collections, up to 1 degree up the inheritance chain).
  • The updated logic is simplified and made more performant in this PR by obtaining all the runtime information in one pass: avoiding repeated checks for converters, element types, class types, and removing complex logic involved in assigning converters to types. This logic is expected to now be more inclusive, and accounts for previously held assumptions.

Serialization support

Beside removing checks for the IDictionaryConstructible class types (which are now regarded simply as Dictionary types), this change doesn't modify the serialization code paths, so it's unlikely for the serializer to stop supporting any previously supported types.

Risk

The risk of this PR is described in the regression section. I'll be paying close attention to incoming issues to mitigate as needed.

)

* Populate non-immutable collections directly on deserialize

(rather than storing in temporary collections and using converters to
create and populate instances)

* Fix deserilizing nested dictionaries

* Don't get add method for types we can populate without reflection; fix failing tests

* Misc perf changes

* Additional changes

* More changes

* Address review feedback

* Address feedback
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Text.Json Servicing-consider Issue for next servicing release review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants