EnumerableSerializeFactory fixes #81

joey-becker · 2017-12-05T16:36:50Z

Update this factory to handle classes that implement IEnumerable<T> and have a constructor that accepts IEnumerable<T> (e.g. FSharpSet<T> and LinkedList<T>) and to use compiled expressions instead of invoking MethodInfo for Add and AddRange.

Horusiath

This is nice idea (checking for constructor with IEnumerable<> arg) 👍 However, there are several concerns to deal with.

Horusiath · 2017-12-05T18:28:05Z

Hyperion/SerializerFactories/EnumerableSerializerFactory.cs

-                var instance = Activator.CreateInstance(type);
-                if (preserveObjectReferences)
+                var count = stream.ReadInt32(session);
+                if (construct != null)


You can move those null checks outside of the ObjectReader implementation - it's not like the constructor/addRange/add presence for the same type will change from one deserializer call to another. Instead you can make those checks outside and return one of the 3 different ObjectReader implementations (depending if constructor/addRange/add is available).

Horusiath · 2017-12-05T18:31:31Z

Hyperion/SerializerFactories/EnumerableSerializerFactory.cs

+              : null;
+        }
+
+        private static Func<object, object> CompileCtorToDelegate(ConstructorInfo ctor, Type argType)


We need to check which is faster: using lambda Func<,> which call the constructor, or calling ConstructorInfo.Invoke directly and use that one.

+1 Is there any standards for benchmarking I should follow, e.g. which benchmarking library to use, what data to test on, etc.?

Horusiath · 2017-12-05T18:32:44Z

Hyperion/SerializerFactories/EnumerableSerializerFactory.cs

+            return compiled;
+        }
+
+        private static Action<object, object> CompileMethodToDelegate(MethodInfo method, Type instanceType, Type argType)


Same as with calling ConstructorInfo.Invoke vs Func<,>.

Horusiath · 2017-12-05T18:35:40Z

Hyperion/SerializerFactories/EnumerableSerializerFactory.cs

                }

-                var count = stream.ReadInt32(session);
-
+                var instance = Activator.CreateInstance(type);


If you're going to call Activator.CreateInstance make sure, that you've checked that in CanSerialize you've checked that this type has default constructor. Also in hyperion we accept calling non-public constructors, so Activator.CreateInstance(type, true) is probably better and more explicit in this context.

Good catch on using Activator.CreateInstance(type, true). On a similar note, should I provide the appropriate BindingFlags value to type.GetTypeInfo().GetConstructor to return non-public constructors there as well?

Horusiath · 2017-12-05T20:08:25Z

We're going to move to BenchmarkDotNet with performance benchmarks(see #57). Also I've made initial benchmarks, you can see them here. General results are:

BenchmarkDotNet=v0.10.10, OS=Windows 10 Redstone 2 [1703, Creators Update] (10.0.15063.726)
Processor=Intel Core i5-4430 CPU 3.00GHz (Haswell), ProcessorCount=4
Frequency=2929686 Hz, Resolution=341.3335 ns, Timer=TSC
.NET Core SDK=2.1.1
  [Host]     : .NET Core 2.0.3 (Framework 4.6.25815.02), 64bit RyuJIT
  DefaultJob : .NET Core 2.0.3 (Framework 4.6.25815.02), 64bit RyuJIT

Method	Mean	Error	StdDev	Min	Max	Op/s	Gen 0	Allocated
ConstructorInfo_Invoke	337.8 ns	1.9053 ns	1.7822 ns	334.5 ns	340.3 ns	2,960,186.6	0.0558	176 B
LambdaConstructor_Call	101.5 ns	0.4284 ns	0.3798 ns	100.8 ns	102.1 ns	9,853,714.5	0.0355	112 B
Activator_plus_AddRange_Info	395.3 ns	0.6649 ns	0.5894 ns	394.0 ns	396.1 ns	2,529,865.9	0.0558	176 B
Activator_plus_Add_Info	1,473.4 ns	6.8928 ns	6.4475 ns	1,463.3 ns	1,489.2 ns	678,701.8	0.2251	712 B
Activator_plus_AddRange_Lambda	396.4 ns	1.1750 ns	1.0991 ns	394.7 ns	398.7 ns	2,522,868.7	0.0558	176 B
Activator_plus_Add_Lambda	1,480.1 ns	3.2393 ns	2.8715 ns	1,474.7 ns	1,484.4 ns	675,621.2	0.2251	712 B

Summary:

Calling constructor composed as Func<object,object> is hands down the fastest, also faster and lighter than calling ConstructorInfo.Invoke.
When constructing via Activator.CreateInstance+MethodInfo vs. using Activator.CreateInstance+Action<object,object> there's almost no difference.

…ader, allow private constructors.

Horusiath

My mistake in benchmarks - when we are creating a new collection via constructor with IEnumerable<> argument, we also need to add a cost of initializing an array. I'll make updated benchmarks later today.

Horusiath · 2017-12-06T16:21:58Z

Hyperion/SerializerFactories/EnumerableSerializerFactory.cs

@@ -48,6 +51,8 @@ public override bool CanSerialize(Serializer serializer, Type type)
            return false;
        }

+        private static readonly BindingFlags allInstanceBindings = BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance;


There is a BindingFlagsEx.All member, which does exactly that ;)

joey-becker · 2017-12-07T16:03:21Z

I updated the benchmarks, ran them, and put the code/results here. Please let me know if everything looks OK to you.

I updated the constructor and add range benchmarks to use Array.CreateInstance since that is what happens in the ObjectReader for each.

Also, the Activator_plus_AddRange_Lambda and Activator_plus_Add_Lambda methods weren't actually calling the lambda, so I updated those as well. As you can see, this significantly reduced the mean time for each, especially ine the case of Activator_plus_Add_Lambda.

Based on these results, it seems that calling Array.CreateInstance and items.SetValue has a significant impact. Do you think we should refactor to avoid working with the Array base class?

BenchmarkDotNet=v0.10.11, OS=Windows 10 Redstone 2 [1703, Creators Update] (10.0.15063.674)
Processor=Intel Core i5-4300M CPU 2.60GHz (Haswell), ProcessorCount=4
Frequency=2533210 Hz, Resolution=394.7561 ns, Timer=TSC
  [Host]     : .NET Framework 4.6.1 (CLR 4.0.30319.42000), 32bit LegacyJIT-v4.7.2115.0
  DefaultJob : .NET Framework 4.6.1 (CLR 4.0.30319.42000), 32bit LegacyJIT-v4.7.2115.0

Method	Mean	Error	StdDev	Min	Max	Op/s	Gen 0	Allocated
ConstructorInfo_Invoke	991.9 ns	8.2902 ns	7.3490 ns	982.2 ns	1,008.5 ns	1,008,197.2	0.1717	272 B
LambdaConstructor_Call	567.4 ns	1.8830 ns	1.7614 ns	564.7 ns	569.4 ns	1,762,474.1	0.1516	240 B
Activator_plus_AddRange_Info	1,116.0 ns	5.6749 ns	5.0307 ns	1,109.6 ns	1,127.5 ns	896,059.0	0.2098	332 B
Activator_plus_Add_Info	1,794.8 ns	7.6771 ns	7.1812 ns	1,785.7 ns	1,809.1 ns	557,154.9	0.2728	432 B
Activator_plus_AddRange_Lambda	695.5 ns	2.1071 ns	1.9709 ns	692.2 ns	698.9 ns	1,437,889.1	0.1898	300 B
Activator_plus_Add_Lambda	194.6 ns	0.6606 ns	0.5856 ns	193.6 ns	195.8 ns	5,138,565.7	0.1523	240 B

Horusiath · 2017-12-07T19:44:15Z

From the benchmarks it looks like by default the best option would be an Activator.CreateInstance + Add lambda. Maybe just make a special case for stacks, that will use constructor with arguments?

joey-becker · 2017-12-07T20:40:32Z

Just to make sure I understand your suggestion correctly, should we add a special case for Stack<T> that either reverses the order of its elements either before they are serialized or after they are deserialized into an Array, or should we let the default ObjectSerializer handle Stack<T> (as is the case in the current version)?

Also, for other types, we should look for an Add method (since it's the fastest), the look for a constructor from an IEnumerable<T> (since it's the second fastest), and then finally look for an AddRange method (since it's the slowest). Is that what you had in mind? Or should we maybe skip the check for AddRange since a class that doesn't have an Add method is unlikely to have an AddRange method?

Horusiath · 2017-12-08T20:15:46Z

What I meant, was to create ObjectReader based on:

If type is a Stack<T> or if it doesn't has default constructor, but has constructor with IEnumerable<T>, then initialize it using that constructor.
If type has default constructor and Add method taking element T as its only argument, then use Activator.CreateInstance<T> and Add method for every deserialized element. This is the fastest step.
In the remaining cases we probably don't know how to serialize that type, so just use NotSupportedSerializer.

What do you think about that?

joey-becker · 2017-12-08T20:55:26Z

I tried using the IEnumerable<T> constructor for Stack<T>, but it fails some of the roundtrip tests because for a given Stack<T> stack, the expression new Stack<T>(stack) returns a Statck<T> whose elements are in reverse order of stack's.

In the current version of the code, EnumerableSerializerFactory.CanSerialize returned false when type is Stack<T>, so the it defaults to a plain ObjectSerializer.

So which of these two options do you think would be the better one:

Update CanSerialize to return true for Stack<T> and handle the ordering issue when serializing or deserializing, or
Continue to return false from CanSeralize for Stack<T>?

…Enumerable<T> ctor.

EnumerableSerializeFactory fixes

301b4cf

Horusiath requested changes Dec 5, 2017

View reviewed changes

Update EnumerableSerializerFactory. Move null checks outside ObjectRe…

9c90456

…ader, allow private constructors.

Horusiath reviewed Dec 7, 2017

View reviewed changes

Merge branch 'dev' into dev

08d6d5e

joey-becker added 2 commits December 21, 2017 09:57

Update EnumerableSerializerFactor to use Add method if exists, then I…

109e2b9

…Enumerable<T> ctor.

Merge branch 'dev' of https://github.com:/joey-becker/Hyperion into dev

f345f93

Horusiath approved these changes Dec 21, 2017

View reviewed changes

Horusiath merged commit 850fe15 into akkadotnet:dev Jan 8, 2018

Aaronontheweb added this to the Hyperion v0.9.7 milestone Jan 18, 2018

This was referenced Jan 18, 2018

Hyperion v0.9.7 release notes [dev] #87

Merged

Hyperion v0.9.7 production release #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EnumerableSerializeFactory fixes #81

EnumerableSerializeFactory fixes #81

joey-becker commented Dec 5, 2017 •

edited

Loading

Horusiath left a comment

Horusiath Dec 5, 2017

Horusiath Dec 5, 2017

joey-becker Dec 5, 2017

Horusiath Dec 5, 2017

Horusiath Dec 5, 2017

joey-becker Dec 5, 2017

Horusiath commented Dec 5, 2017

Horusiath left a comment

Horusiath Dec 6, 2017

joey-becker commented Dec 7, 2017

Horusiath commented Dec 7, 2017

joey-becker commented Dec 7, 2017

Horusiath commented Dec 8, 2017 •

edited

Loading

joey-becker commented Dec 8, 2017 •

edited

Loading

EnumerableSerializeFactory fixes #81

EnumerableSerializeFactory fixes #81

Conversation

joey-becker commented Dec 5, 2017 • edited Loading

Horusiath left a comment

Choose a reason for hiding this comment

Horusiath Dec 5, 2017

Choose a reason for hiding this comment

Horusiath Dec 5, 2017

Choose a reason for hiding this comment

joey-becker Dec 5, 2017

Choose a reason for hiding this comment

Horusiath Dec 5, 2017

Choose a reason for hiding this comment

Horusiath Dec 5, 2017

Choose a reason for hiding this comment

joey-becker Dec 5, 2017

Choose a reason for hiding this comment

Horusiath commented Dec 5, 2017

Horusiath left a comment

Choose a reason for hiding this comment

Horusiath Dec 6, 2017

Choose a reason for hiding this comment

joey-becker commented Dec 7, 2017

Horusiath commented Dec 7, 2017

joey-becker commented Dec 7, 2017

Horusiath commented Dec 8, 2017 • edited Loading

joey-becker commented Dec 8, 2017 • edited Loading

joey-becker commented Dec 5, 2017 •

edited

Loading

Horusiath commented Dec 8, 2017 •

edited

Loading

joey-becker commented Dec 8, 2017 •

edited

Loading