Add support for more collections#38319
Conversation
…takes an IEnumerable or IDictionary
steveharter
left a comment
There was a problem hiding this comment.
Main feedback:
- Custom converter support can be used for KeyValuePair in the near future (still in V3)
- Can we use
propertyinfo<T>to speed up instantiation of dictionary classes? - There is too much new stack frame state being added. The code is becoming hard to understand. This needs to be cleaned up IMO (future); we need to create custom converters that understand our custom stack \ state machine.
| { | ||
| internal sealed class DefaultICollectionConverter : JsonEnumerableConverter | ||
| { | ||
| public override IEnumerable CreateFromList(ref ReadStack state, IList sourceList, JsonSerializerOptions options) |
There was a problem hiding this comment.
Ideally the enumerable converter code here can avoid Activator.CreateInstance and instead call a helper method on the propertyinfo<T> classes to create one quicker using the existing generic support. It would be good to measure the benchmark of Activator.CreateInstance vs. new Foo<T>.
There was a problem hiding this comment.
This gives us the expected perf benefit.
FYI Activator.CreateInstance is a fallback for non-generic Stack, Queue, and SortedList at the moment. I wasn't sure if I could add a ref to System.Collections.Generic so I could new these up directly.
I'll circle back and modify this if the ref is okay. (So no perf benefit for Stack, Queue, SortedList right now.) Reflection can be explored also.
Before
| Method | ElementCount | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|
| DeserializeStack | 2 | 2.230 us | 0.0440 us | 0.0433 us | 0.3128 | - | - | 1320 B |
| DeserializeQueue | 2 | 2.372 us | 0.0447 us | 0.0396 us | 0.3052 | - | - | 1288 B |
| DeserializeArrayList | 2 | 2.155 us | 0.0415 us | 0.0408 us | 0.3128 | - | - | 1312 B |
| DeserializeGenericStack | 2 | 1.437 us | 0.0324 us | 0.0494 us | 0.2098 | - | - | 880 B |
| DeserializeGenericQueue | 2 | 1.424 us | 0.0158 us | 0.0123 us | 0.2117 | - | - | 888 B |
| DeserializeGenericHashSet | 2 | 1.760 us | 0.0174 us | 0.0146 us | 0.2766 | - | - | 1160 B |
| DeserializeGenericLinkedList | 2 | 1.344 us | 0.0423 us | 0.0396 us | 0.2136 | - | - | 896 B |
| DeserializeGenericSortedSet | 2 | 1.943 us | 0.0307 us | 0.0287 us | 0.2556 | - | - | 1072 B |
| DeserializeHashtable | 2 | 3.224 us | 0.0636 us | 0.1027 us | 0.4768 | - | - | 2000 B |
| DeserializeSortedList | 2 | 3.330 us | 0.0659 us | 0.1172 us | 0.4005 | - | - | 1688 B |
| DeserializeStack | 50 | 29.758 us | 0.5153 us | 0.4820 us | 3.8452 | - | - | 16161 B |
| DeserializeQueue | 50 | 30.277 us | 0.6048 us | 0.8865 us | 3.8452 | 0.0610 | - | 16193 B |
| DeserializeArrayList | 50 | 29.323 us | 0.5716 us | 0.7229 us | 3.9673 | - | - | 16601 B |
| DeserializeGenericStack | 50 | 8.624 us | 0.1625 us | 0.1872 us | 1.2817 | - | - | 5384 B |
| DeserializeGenericQueue | 50 | 8.582 us | 0.1633 us | 0.1747 us | 1.2817 | - | - | 5392 B |
| DeserializeGenericHashSet | 50 | 10.458 us | 0.1476 us | 0.1233 us | 1.8616 | - | - | 7817 B |
| DeserializeGenericLinkedList | 50 | 8.748 us | 0.1699 us | 0.1589 us | 1.5869 | - | - | 6648 B |
| DeserializeGenericSortedSet | 50 | 30.077 us | 0.1441 us | 0.1348 us | 1.8616 | - | - | 7880 B |
| DeserializeHashtable | 50 | 40.280 us | 0.3471 us | 0.3077 us | 5.6763 | 0.1221 | - | 23945 B |
| DeserializeSortedList | 50 | 53.019 us | 0.4307 us | 0.4029 us | 5.4321 | 0.4883 | - | 22734 B |
| DeserializeStack | 100 | 57.856 us | 0.6680 us | 0.5922 us | 7.5684 | 0.5493 | - | 31705 B |
| DeserializeQueue | 100 | 57.985 us | 0.6973 us | 0.6523 us | 7.5684 | 0.7324 | - | 31737 B |
| DeserializeArrayList | 100 | 55.621 us | 0.5674 us | 0.5307 us | 7.7515 | 0.7324 | - | 32545 B |
| DeserializeGenericStack | 100 | 15.228 us | 0.1158 us | 0.1083 us | 2.3804 | - | - | 9977 B |
| DeserializeGenericQueue | 100 | 15.353 us | 0.3068 us | 0.4300 us | 2.3804 | - | - | 9985 B |
| DeserializeGenericHashSet | 100 | 19.531 us | 0.2002 us | 0.1774 us | 3.6621 | 0.0305 | - | 15353 B |
| DeserializeGenericLinkedList | 100 | 15.922 us | 0.1344 us | 0.1122 us | 2.9907 | - | - | 12592 B |
| DeserializeGenericSortedSet | 100 | 63.071 us | 1.3730 us | 2.7102 us | 3.5400 | 0.1831 | - | 14872 B |
| DeserializeHashtable | 100 | 77.650 us | 0.8260 us | 0.7726 us | 11.4746 | 0.1221 | - | 48281 B |
| DeserializeSortedList | 100 | 110.629 us | 2.0643 us | 3.3334 us | 10.8643 | 0.3662 | - | 45706 B |
After
| Method | ElementCount | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|
| DeserializeStack | 2 | 2,325.1 ns | 24.916 ns | 23.306 ns | 0.3471 | - | - | 1456 B |
| DeserializeQueue | 2 | 2,403.6 ns | 15.886 ns | 14.082 ns | 0.3395 | - | - | 1424 B |
| DeserializeArrayList | 2 | 1,553.2 ns | 12.120 ns | 11.338 ns | 0.2174 | - | - | 912 B |
| DeserializeGenericStack | 2 | 803.2 ns | 8.654 ns | 7.671 ns | 0.1278 | - | - | 536 B |
| DeserializeGenericQueue | 2 | 804.9 ns | 9.085 ns | 7.586 ns | 0.1297 | - | - | 544 B |
| DeserializeGenericHashSet | 2 | 835.4 ns | 8.751 ns | 8.186 ns | 0.1488 | - | - | 624 B |
| DeserializeGenericLinkedList | 2 | 784.1 ns | 7.897 ns | 6.594 ns | 0.1392 | - | - | 584 B |
| DeserializeGenericSortedSet | 2 | 1,025.7 ns | 8.090 ns | 7.568 ns | 0.1545 | - | - | 648 B |
| DeserializeHashtable | 2 | 1,896.1 ns | 12.512 ns | 11.704 ns | 0.2937 | - | - | 1232 B |
| DeserializeSortedList | 2 | 3,581.0 ns | 36.296 ns | 30.308 ns | 0.4311 | - | - | 1816 B |
| DeserializeStack | 50 | 28,485.1 ns | 203.798 ns | 180.662 ns | 3.8757 | 0.0305 | - | 16297 B |
| DeserializeQueue | 50 | 29,227.1 ns | 230.634 ns | 204.451 ns | 3.8757 | 0.0305 | - | 16329 B |
| DeserializeArrayList | 50 | 27,774.8 ns | 137.346 ns | 121.754 ns | 3.8452 | 0.0305 | - | 16200 B |
| DeserializeGenericStack | 50 | 7,787.4 ns | 144.107 ns | 134.798 ns | 1.1978 | - | - | 5040 B |
| DeserializeGenericQueue | 50 | 7,946.6 ns | 184.048 ns | 263.956 ns | 1.2054 | - | - | 5048 B |
| DeserializeGenericHashSet | 50 | 9,792.8 ns | 202.762 ns | 189.664 ns | 1.7395 | - | - | 7280 B |
| DeserializeGenericLinkedList | 50 | 8,666.3 ns | 171.621 ns | 262.083 ns | 1.5106 | - | - | 6336 B |
| DeserializeGenericSortedSet | 50 | 29,945.8 ns | 578.101 ns | 593.667 ns | 1.7700 | - | - | 7456 B |
| DeserializeHashtable | 50 | 40,584.1 ns | 762.719 ns | 1,782.832 ns | 5.4932 | 0.1831 | - | 23129 B |
| DeserializeSortedList | 50 | 54,203.1 ns | 1,038.124 ns | 920.270 ns | 5.4321 | 0.1831 | - | 22854 B |
| DeserializeStack | 100 | 57,629.2 ns | 1,108.796 ns | 1,232.424 ns | 7.5684 | 0.0610 | - | 31849 B |
| DeserializeQueue | 100 | 58,428.0 ns | 976.921 ns | 913.813 ns | 7.5684 | 0.9155 | - | 31881 B |
| DeserializeArrayList | 100 | 54,157.4 ns | 1,075.539 ns | 1,104.500 ns | 7.6294 | 0.2441 | - | 32152 B |
| DeserializeGenericStack | 100 | 14,738.4 ns | 274.104 ns | 242.986 ns | 2.3041 | - | - | 9640 B |
| DeserializeGenericQueue | 100 | 14,913.8 ns | 296.208 ns | 405.453 ns | 2.3041 | - | - | 9648 B |
| DeserializeGenericHashSet | 100 | 18,173.4 ns | 340.586 ns | 318.585 ns | 3.5400 | - | - | 14824 B |
| DeserializeGenericLinkedList | 100 | 15,275.7 ns | 305.165 ns | 326.523 ns | 2.9297 | - | - | 12288 B |
| DeserializeGenericSortedSet | 100 | 63,097.3 ns | 1,250.131 ns | 2,088.688 ns | 3.4180 | - | - | 14456 B |
| DeserializeHashtable | 100 | 75,520.9 ns | 1,489.822 ns | 1,529.938 ns | 11.3525 | 0.1221 | - | 47507 B |
| DeserializeSortedList | 100 | 110,677.6 ns | 1,929.695 ns | 1,710.624 ns | 10.8643 | 0.2441 | - | 45843 B |
Agreed.
Done. This gives the expected perf gain: #38319 (comment)
Agreed. KeyValuePair support pushes it over the edge. Creating a tracking issue for the refactoring: https://github.com/dotnet/corefx/issues/38552. |
|
Hot-path (de)serialization cases - no significant perf change: summary:
|
* Add support for ISet * Add support for IEnumerable, IList, and ICollection (non-generic) * Add support for non-generic IDictionary * Address review feedback * Remove unused exception resource and remove ISet<ISet<T>> (de)serialization limitation * Add support for non-generic collections that have a constructor that takes an IEnumerable or IDictionary * Condense branches for types we need to set runtime types for * Add support for non-generic collections that have a constructor that takes an IList * Add support for Hashtable and SortedList * Verify sorted dictionary works * Add support for KeyValuePair * Create concrete instance of types Foo where possible * Use generic constructor for KeyValuePair * Fix KeyValuePair creation in tests Commit migrated from dotnet/corefx@4aece7a
Partially addresses https://github.com/dotnet/corefx/issues/36643.
Specifically:
ISet<T>IEnumerable, ICollection, IListIDicitonaryStack,Queue,ArrayListHashtable,SortedListKeyValuePair<string, TValue>Will follow up with perf notes.