Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/3.1] Port polymorphic perf and better dictionary support #42008

Closed

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Oct 22, 2019

Update: the fixes for these two issues now exist in PRs #42057 and #42050 since these do not take the dependency on #42009 which may not be accepted.

note: the branch here is based on @layomia current branch for another issue which is not yet in 3.1 so you can ignore the first commit here. The next two commits handle two separate issues but since they conflict, just one PR:

Ports the following to release/3.1
#41753
#41903

Issues:

  • Significant performance improvement when using polymorphic serialization.
  • Support dictionaries that implement IDictionary<TKey, TValue> but do not implement the non-generic IDictionary

Description

  • When a property of type of System.Object is serialized, the serializer enters a polymorphic mode where it obtains the actual run-time type to serialize. Getting the appropriate infrastructure (including looking up a converter for that type and generating IL for the setter) is expensive and was not cached. This PR caches that information. The issue was created and fixed by a community member by adding a dictionary-based cache.
  • An assumption was made during serialization that a call to a dictionary's GetEnumerator() (either through the IEnumerable or IDictionary interfaces) returns an IDictionaryEnumerable as required for types that implement IDictionary. However, there are certain "newer" generic collection types outside of the BCL that don't implement IDictionary or return IDictionaryEnumerable from their IEnumerable.GetEnumerator() method (note that IDictionary<TKey, TValue> does not implement IDictionary). This caused a InvalidCastException at runtime. The fix is to assume IEnumerable<KeyValuePair<TKey, TValue>> instead of IDictionaryEnumerable for generic dictionaries.

Customer Impact

  • Slow behavior when using polymorphic serialization.
  • Lack of support for newer generic dictionaries that don't support the older IDictionary.

Regression?

No. For the dictionary issue, the InvalidCastException is no longer encountered.

Risk

Low.
The performance issue is very specific to polymorphic serialization and was already tested. An additional test was added to verify the cache is properly keying.

For the dictionary issue, existing tests were not modified and new tests added that only implement the generic IDictionary<TKey, TValue>.

layomia and others added 3 commits October 22, 2019 14:51
)

* 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
* Cache polymorphic properties

* Move RuntimePropertyCache to JsonClassInfo

* Added test of RuntimePropertyCache using properties with different attributes

* Fixed typo

Co-Authored-By: Ahson Khan <ahkha@microsoft.com>

* Use allocating overload of GetOrAdd on .Net Standard 2.0
@steveharter steveharter added * NO SQUASH * The PR should not be squashed area-System.Text.Json labels Oct 22, 2019
@steveharter steveharter added this to the 3.1 milestone Oct 22, 2019
@steveharter steveharter self-assigned this Oct 22, 2019
@stephentoub stephentoub changed the title Port perf and dict issue to31 [release/3.1] Port perf and dict issue to31 Oct 23, 2019
@steveharter steveharter changed the title [release/3.1] Port perf and dict issue to31 [release/3.1] Port polymorphic perf and better dictionary support Oct 23, 2019
@steveharter steveharter added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 23, 2019
@steveharter
Copy link
Member Author

Closing since replaced by the two individual PRs mentioned in the description.

@steveharter steveharter deleted the PortPerfAndDictIssueTo31 branch October 23, 2019 20:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) * NO SQUASH * The PR should not be squashed
Projects
None yet
2 participants