Improve data contract serializers performance in dictionary scenario #7608

khdang · 2016-04-08T23:43:59Z

Improve data contract serializers performance in dictionary scenario by 2x.
Instead of the current way to get encoded string which involves string creation and buffer copy, use the encoding.GetBytes API
When creating a new instance of a type without a default constructor we use reflection to invoke a method that is not from the serialization assembly. Since that is significantly slower, we should use Activator.CreateInstance whenever we can. The Activator.CreateInstance throws exception so we need to check to know if a type has default constructor or not and use the appropriate method. As this check is also slow, so I've added a cache to make sure we only do this once for each type.
~~Internally dictionaries are handled as collection of KeyValues. I've updated this type to have default constructor so we can use Activator.CreateInstance to create new instance of it.~~

Update:

Instead of converting KeyValue type to class, which potentially causes GC long pauses, use Activator.CreateInstance for value type

Fix #7578 and #7577

danmoseley · 2016-04-09T00:01:52Z

...Private.DataContractSerialization/src/System/Runtime/Serialization/CollectionDataContract.cs

@@ -68,14 +68,16 @@ public T Value

    [DataContract(Namespace = "http://schemas.microsoft.com/2003/10/Serialization/Arrays")]
 #if USE_REFEMIT
-    public struct KeyValue<K, V>
+    public class KeyValue<K, V>
 #else


why a change from struct? (not saying it's wrong, I just don't follow)

The current issue with dictionary is that they are handled as collection of KeyValue internally and as of now, KeyValue don't have default constructor so they're not created by the faster code path. I added a default constructor but since struct is not allowed to have explicit parameterless constructor, I've changed this type to class.

I'm concerned on this as it diverges from desktop .NET. We'd better avoid it unless absolutely necessary. How much performance gain would this change give us?

This change and the caching of the map indicate type has a default constructor or not give 1.75x improvement for the dictionary scenario. Any specific concern on this change?

What's the perf gain without changing from struct to class?

This is part of the significant improvement in this PR and benefits mainly the dictionary scenario. Other are the change to use encoding.GetBytes which improve just a few percentage, and caching instance of the GetUninitializedObject to avoid repeated reflection which already improves a lot, I recall close to 50%. With this change the performance is at the same bar or better than Desktop in dictionary.

I'm concerned on this as it diverges from desktop .NET.

If the change proves to be beneficial to desktop, we should port the fix to desktop too.

Agree with @zhenlan. I'm curious about the perf difference between desktop and .NET core after this change were made on both. There may be some hotspots not yet identified. LGTM.

I forgot to mention that this change is for NetCore only since this is the part where implementation on Desktop and NetCore diverges. In Desktop we use RuntimeServices.GetUninitlaizedObject to create new object instance whereas in NetCore that API is not available. We opt for getting that method through reflection and this perf issue is incurred.

khdang · 2016-04-09T05:52:34Z

Test Innerloop Windows_NT Debug Build and Test please

khdang · 2016-04-10T21:17:51Z

@dotnet-bot test this please

khdang · 2016-04-10T22:49:57Z

@dotnet-bot test Innerloop Windows_NT Debug Build and Test please

shmao · 2016-04-11T23:02:15Z

...ivate.DataContractSerialization/src/System/Runtime/Serialization/XmlFormatReaderGenerator.cs

@@ -30,6 +30,19 @@ public sealed class XmlFormatReaderGenerator
    internal sealed class XmlFormatReaderGenerator
 #endif
    {
+        private static MethodInfo _getUninitializedObjectMethodInfo;


s_getUninitializedObjectMethodInfo?

Yes that's the right name for private static.

mconnew · 2016-04-12T23:02:44Z

...Private.DataContractSerialization/src/System/Runtime/Serialization/CollectionDataContract.cs

 #else
-    internal struct KeyValue<K, V>


Changing KeyValue from a struct to a class will have a significant performance hit which you won't see in a micro benchmark. This could place a lot of stress on the heap and cause long GC pauses some time later. It will could also cause significantly slower memory accesses as an array of structs are next to each other in memory. An array of classes will require an extra dereference which will be slower, and you won't benefit from cache locality and on high core machines under sufficient load to context switch, this could be a significant issue.

Good point. I've reverted the change of KeyValue back to struct to avoid potential GC impact. I tried a few alternatives to this and realized that Activator.CreateInstance on a struct type uses a special IL for struct and so just calling Activator.CreateInstance will work. I get the numbers for this change and they are as good as the previous change for dictionary scenario.

khdang · 2016-04-14T17:48:06Z

I've updated the PR to address comments on GC and formatting. Can you take a look?
cc: @shmao @mconnew

shmao · 2016-04-15T00:26:10Z

...ivate.DataContractSerialization/src/System/Runtime/Serialization/XmlFormatReaderGenerator.cs

+            if (type.GetTypeInfo().IsValueType)
+            {
+                  return Activator.CreateInstance(type);
+            }


Nit: New line after }

shmao · 2016-04-15T00:37:56Z

mconnew · 2016-04-16T00:50:24Z

justinvp · 2016-04-16T22:16:43Z

...ivate.DataContractSerialization/src/System/Runtime/Serialization/XmlFormatReaderGenerator.cs

+        private static MethodInfo s_getUninitializedObjectMethodInfo;
+        private static Dictionary<Type, bool> s_typeHasDefaultConstructorMap;
+
+        static XmlFormatReaderGenerator()


This explicit static cctor causes the C# compiler to not mark the type as beforefieldinit, which in turn means that the backend compiler needs to add checks to static methods to ensure that the type has been initialized. See #6016 for details.

To avoid this, consider removing the static cctor and initializing the static fields directly, e.g.:

private static readonly MethodInfo s_getUninitializedObjectMethodInfo = typeof(string) .GetTypeInfo() .Assembly .GetType("System.Runtime.Serialization.FormatterServices") ?.GetMethod("GetUninitializedObject", BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static); private static readonly Dictionary<Type, bool> s_typeHasDefaultConstructorMap = new Dictionary<Type, bool>();

Also, these static fields can be readonly.

Thanks for pointing this out! I have removed the static constructor.

…ng changes

khdang · 2016-04-18T18:44:00Z

@dotnet-bot test this please

khdang · 2016-04-18T20:03:46Z

@dotnet-bot test Innerloop Ubuntu Release Build and Test please

khdang · 2016-04-18T21:23:35Z

@dotnet-bot test Innerloop OSX Debug Build and Test please
@dotnet-bot test Innerloop Ubuntu Release Build and Test please

mconnew · 2016-04-19T00:03:38Z

Did you try replacing the MethodInfo.Invoke call with creating a delegate and calling that instead?

khdang · 2016-04-19T19:37:30Z

Yes I did. I was experimenting to get the perf numbers for different changes and wasn't sure how to add that change. I've now updated to include it as well.

justinvp · 2016-04-19T19:47:29Z

...ivate.DataContractSerialization/src/System/Runtime/Serialization/XmlFormatReaderGenerator.cs

@@ -30,6 +30,17 @@ public sealed class XmlFormatReaderGenerator
    internal sealed class XmlFormatReaderGenerator
 #endif
    {
+        private static readonly MethodInfo s_getUninitializedObjectMethodInfo =


Could this field be removed now, replaced by the delegate field?

Yes it can. I was thinking the line may get too long but actually it looks Ok.

justinvp · 2016-04-19T20:42:51Z

...ivate.DataContractSerialization/src/System/Runtime/Serialization/XmlFormatReaderGenerator.cs

-                {
-                    obj = methodInfo.Invoke(null, new object[] { type });
-                }
+                obj = s_getUninitializedObjectDelegate(type);


Sorry for not pointing this out earlier, but this whole method could be simplified as:

static internal object TryGetUninitializedObjectWithFormatterServices(Type type) => s_getUninitializedObjectDelegate?.Invoke(type);

Nice. Updated as suggestion.

…Invoke

khdang · 2016-04-21T18:06:53Z

Test Innerloop CentOS7.1 Debug Build and Test please

stephentoub · 2016-04-22T11:12:11Z

Test Innerloop OSX Debug Build and Test please
Test Innerloop OSX Release Build and Test please

…on_perf_dcs_dictionary Improve data contract serializers performance in dictionary scenario Commit migrated from dotnet/corefx@0444706

khdang added the area-Serialization label Apr 8, 2016

dnfclas added the cla-already-signed label Apr 8, 2016

dotnet-bot added the 2 - In Progress label Apr 8, 2016

danmoseley reviewed Apr 9, 2016
View reviewed changes

khdang force-pushed the improve_serialization_perf_dcs_dictionary branch from 8e670d4 to 773af28 Compare April 9, 2016 05:33

shmao reviewed Apr 11, 2016
View reviewed changes

khdang force-pushed the improve_serialization_perf_dcs_dictionary branch from 773af28 to 042ed27 Compare April 12, 2016 17:53

mconnew reviewed Apr 12, 2016
View reviewed changes

stephentoub added the netfx-port-consider label Apr 13, 2016

AlexGhiondea added netfx-port-consider and removed netfx-port-consider labels Apr 13, 2016

khdang force-pushed the improve_serialization_perf_dcs_dictionary branch from 148c29a to 243289a Compare April 13, 2016 22:44

shmao reviewed Apr 15, 2016
View reviewed changes

Improve data contract serializers performance in dictionary scenarios

70d9121

khdang force-pushed the improve_serialization_perf_dcs_dictionary branch from 243289a to 905308b Compare April 15, 2016 20:51

justinvp reviewed Apr 16, 2016
View reviewed changes

Fix concerns for GC pause, remove static constructor & minor formatti…

91df870

…ng changes

khdang force-pushed the improve_serialization_perf_dcs_dictionary branch from 905308b to 91df870 Compare April 18, 2016 18:42

justinvp reviewed Apr 19, 2016
View reviewed changes

khdang force-pushed the improve_serialization_perf_dcs_dictionary branch from 25e8738 to fcc71d2 Compare April 19, 2016 20:06

justinvp reviewed Apr 19, 2016
View reviewed changes

Use delegate of GetUninitializedObject instead of calling MethodInfo.…

025779f

…Invoke

khdang force-pushed the improve_serialization_perf_dcs_dictionary branch from fcc71d2 to 025779f Compare April 19, 2016 22:43

khdang merged commit 0444706 into dotnet:master Apr 22, 2016

dotnet-bot removed the 2 - In Progress label Apr 22, 2016

khdang deleted the improve_serialization_perf_dcs_dictionary branch April 25, 2016 17:49

karelz modified the milestone: 1.0.0-rtm Dec 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve data contract serializers performance in dictionary scenario #7608

Improve data contract serializers performance in dictionary scenario #7608

khdang commented Apr 8, 2016

danmoseley Apr 9, 2016

khdang Apr 9, 2016

SGuyGe Apr 11, 2016

khdang Apr 11, 2016

SGuyGe Apr 11, 2016

khdang Apr 11, 2016

zhenlan Apr 11, 2016

SGuyGe Apr 11, 2016

khdang Apr 12, 2016

khdang commented Apr 9, 2016

khdang commented Apr 10, 2016

khdang commented Apr 10, 2016

shmao Apr 11, 2016

khdang Apr 12, 2016

mconnew Apr 12, 2016

khdang Apr 14, 2016

khdang commented Apr 14, 2016

shmao Apr 15, 2016

shmao commented Apr 15, 2016

mconnew commented Apr 16, 2016

justinvp Apr 16, 2016 •

edited

Loading

khdang Apr 18, 2016

khdang commented Apr 18, 2016

khdang commented Apr 18, 2016

khdang commented Apr 18, 2016 •

edited

Loading

mconnew commented Apr 19, 2016

khdang commented Apr 19, 2016

justinvp Apr 19, 2016

khdang Apr 19, 2016

justinvp Apr 19, 2016

khdang Apr 19, 2016

khdang commented Apr 21, 2016

stephentoub commented Apr 22, 2016

Improve data contract serializers performance in dictionary scenario #7608

Improve data contract serializers performance in dictionary scenario #7608

Conversation

khdang commented Apr 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khdang commented Apr 9, 2016

khdang commented Apr 10, 2016

khdang commented Apr 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khdang commented Apr 14, 2016

Choose a reason for hiding this comment

shmao commented Apr 15, 2016

mconnew commented Apr 16, 2016

justinvp Apr 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khdang commented Apr 18, 2016

khdang commented Apr 18, 2016

khdang commented Apr 18, 2016 • edited Loading

mconnew commented Apr 19, 2016

khdang commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khdang commented Apr 21, 2016

stephentoub commented Apr 22, 2016

justinvp Apr 16, 2016 •

edited

Loading

khdang commented Apr 18, 2016 •

edited

Loading