Coreclr compatibility: Implemented Json.Net fallback serializer #1047

dVakulen · 2015-11-19T21:51:05Z

Related to #368.
Default fallback serializer is BinaryFormatter, using of the Json.Net one can be configured through boolean property "UseJsonFallbackSerializer".

gabikliot · 2015-11-20T00:32:03Z

src/OrleansAzureUtils/Providers/Streams/AzureQueue/AzureQueueBatchContainer.cs

@@ -36,8 +37,8 @@ namespace Orleans.Providers.Streams.AzureQueue
    internal class AzureQueueBatchContainer : IBatchContainer
    {
        private EventSequenceToken sequenceToken;
-        private readonly List<object> events;
-        private readonly Dictionary<string, object> requestContext;
+        public readonly List<object> events;


This is going to be completely unmanagable.
If we want internal runtime types to be Json serializable, we need to find a better/robust/testable way to enforce that. Otherwise, it will keep breaking. Look at the 5 times we already broke the pub sub state json serialization, since we used the approach of marking fields public. There are much better alternatives.

I suggest to open a separate issue and discuss it. I am sure people in our community have better expertise how to make sure their internal types are json-able.

Following the way of fixing PubSubStateGrain was quite bad idea.

gabikliot · 2015-11-20T05:53:10Z

src/Orleans/Serialization/SerializationManager.cs

+#if DNXCORE50
+                throw new OrleansException("Can't use binary formatter as fallback serializer while running on .Net Core");
+#else
+                serializer = new BinaryFormatterSerializer();


Does not seem like we need to allocate a new BinaryFormatterSerializer instance every time it is called, right? It is pretty much stateless, just a call through into Ser/Deser/Copy methods. So better allocate once and return, save some GC.
Same applies to OrleansJsonSerializer.
Serialization code is of course the most sensitive to perf, as it is on the hottest path.

dVakulen · 2015-11-20T23:51:47Z

Updated. Made few more internal types json serialization friendly.

gabikliot · 2015-11-21T00:25:17Z

src/Tester/StreamingTests/PullingAgentManagementTests.cs

            }

            var numAgents = await mgmt.SendControlCommandToProvider(adapterType, adapterName, (int)PersistentStreamProviderCommand.GetNumberRunningAgents);
            Assert.AreEqual(2, numAgents.Length);
-            int totalNumAgents = numAgents.Cast<int>().Sum();
+            int totalNumAgents = numAgents.Select(n=>int.Parse(n.ToString())).Sum();


Please remove those n=>int.Parse(n.ToString().
We should not need to go via string.
If there is a problem with int to long, we can fix that.

When serializing with binary formatter the resulting array contains ints and when with Json.Net one - longs.

jason-bragg · 2015-11-21T01:39:23Z

"I ran all tests with json serializer, so that i found the issues with it. Yes, I'll add new tests."

At a quick glance, I'm seeing over a hundred Orleans classes marked [Serializable]. I'd like to believe they are all covered by tests, but I'm not that confident. I don't know any simple way to vet this change.

dVakulen · 2015-11-21T02:29:34Z

BinaryFormatter still remains the default serializer, so this one should be pretty safe. I can remove ability to configure it so that users won't be able to use untested feature, and Json.Net will be used only when running on .Net Core and tested along the other specific to the coreclr changes.

dVakulen · 2015-11-21T02:47:55Z

Commits squashed.

dVakulen · 2015-11-21T18:23:37Z

Removed some allocations in the serialization code using thread local serializers instead of creation of the new ones on each serialization\deserialization, the number of GC collections of the zero gen on benchmark with deep copy of POCO decreased by ~7% but performance for binary serialization for some reason dropped by 15%. Will create separate PR for this later, if needed. Also, seems like Json.Net 2 - 3 times faster than binary formatter and causes lower GC pressure (2x less collections of the zero gen in comparison to the BinaryFormatter, 4x - first gen)

gabikliot · 2015-11-25T05:29:49Z

src/Orleans/Serialization/SerializationManager.cs

+                    "Cann't use binary formatter as fallback serializer while running on .Net Core, will use Json.Net instead");
+            }
+
+            useJsonFallbackSerializer = true;


this.useJsonFallbackSerializer = true, right?

No, i replaced field useJsonFallbackSerializer with IExternalSerializer fallbackSerializer.

gabikliot · 2015-11-25T05:32:43Z

OK, so I have a couple of comments:

In MessagingConfiguration you also need to print the new property UseJsonFallbackSerializer
ISerializer: does not feel like we really need a new interface ISerializer. Lets just use the same one IExternalSerializer and we can later rename the IExternalSerializer to ISerializer. And IsSupportedType can just return true in the fallback one.
Lets try to minimize/standardize the way we integrate with Json on internal runtime types. So for example, for ActivationAddress and GuidId and UniqueIdentifier lets use the converter, like you did for IPAddress inside the JsonSerializer, instead of JsonProperty and JsonConstructor.
AzureQueueBatchContainer can not be specified in the JsonSerializer, since it will create a dependancy on Azure dll, therefore using JsonProperty or ISerializable interface is OK.
After all fixes, please rebase and squash into one commit.

I would like this PR to be accepted after #1052.

dVakulen · 2015-11-25T21:01:24Z

Updated & Squashed. Left JsonConstructor on ActivationAddress and JsonProperty in UniqueIdentifier because implementing converters for them would require too much seemingly unnecessary code compared to just adding attributes.

gabikliot · 2015-11-26T05:35:17Z

Looks good. Going to merge.

There is still an open question if all our internal types are json-serializable or do we miss any.
There is a good chance we are indeed missing something, like @jason-bragg pointed above in his comment: #1047 (comment).
I agree with @dVakulen that BinaryFormatter still remains the default serializer, so this one should be pretty safe and for now only intended to be used with .Net Core. As we use more and more json, we will discover those gaps and fill them in.

Implemented Json.Net fallback serializer

gabikliot · 2015-11-26T05:37:18Z

Thank you @dVakulen !

sergeybykov · 2015-11-26T05:42:23Z

I planned to merge it post 1.1.0. That's why I assigned the vNext milestone to it.

gabikliot · 2015-11-26T05:49:46Z

Ohhhh...
Sorry.
It was not clear to me. I thought we said we will mark it as On Hold, as we said before, with label or milestone. I did not understand what vNext meant - I thought it is like coreclr - vNextDotNet.

Anyway, I think this PR was safe. It added json as an alternative fallback serializer, but the binary serializer was still the default one, and it did not change.

dVakulen · 2015-11-26T07:06:19Z

@gabikliot Thank you for the review!

dnfclas added the cla-already-signed label Nov 19, 2015

dVakulen force-pushed the json-net-fallback-serializer branch 3 times, most recently from 987fb9d to 4bef196 Compare November 19, 2015 22:19

gabikliot reviewed Nov 20, 2015
View reviewed changes

gabikliot added the enhancement label Nov 20, 2015

gabikliot reviewed Nov 20, 2015
View reviewed changes

dVakulen force-pushed the json-net-fallback-serializer branch from 140bb89 to a4e0006 Compare November 20, 2015 23:51

gabikliot reviewed Nov 21, 2015
View reviewed changes

dVakulen force-pushed the json-net-fallback-serializer branch from a4e0006 to 3e158f1 Compare November 21, 2015 02:47

dVakulen force-pushed the json-net-fallback-serializer branch 3 times, most recently from 5790bac to b222230 Compare November 21, 2015 05:48

gabikliot mentioned this pull request Nov 21, 2015

Determine the order of serializers. #1052

Merged

dVakulen force-pushed the json-net-fallback-serializer branch 3 times, most recently from bf78197 to b222230 Compare November 21, 2015 20:14

dVakulen mentioned this pull request Nov 21, 2015

Problem with PubSubStateGrain JSON serialization #1041

Closed

sergeybykov assigned gabikliot Nov 24, 2015

gabikliot reviewed Nov 25, 2015
View reviewed changes

sergeybykov added this to the vNext milestone Nov 25, 2015

Implemented Json.Net fallback serializer

06b86c1

dVakulen force-pushed the json-net-fallback-serializer branch from b222230 to 06b86c1 Compare November 25, 2015 20:50

gabikliot pushed a commit that referenced this pull request Nov 26, 2015

Merge pull request #1047 from dVakulen/json-net-fallback-serializer

ae97c47

Implemented Json.Net fallback serializer

gabikliot merged commit ae97c47 into dotnet:master Nov 26, 2015

gabikliot modified the milestones: CoreCLR, vNext Nov 26, 2015

gabikliot mentioned this pull request Nov 26, 2015

Contribution idea: Port Orleans to coreclr #368

Closed

30 tasks

gabikliot changed the title ~~Implemented Json.Net fallback serializer~~ Coreclr compatibility: Implemented Json.Net fallback serializer Nov 26, 2015

dVakulen deleted the json-net-fallback-serializer branch November 26, 2015 07:12

gabikliot modified the milestones: CoreCLR, v1.2.0 Dec 4, 2015

gabikliot mentioned this pull request Jan 12, 2016

Bug in Json.Net: System.Int32 is deserialized as System.Int64 #1269

Closed

github-actions bot locked and limited conversation to collaborators Dec 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coreclr compatibility: Implemented Json.Net fallback serializer #1047

Coreclr compatibility: Implemented Json.Net fallback serializer #1047

dVakulen commented Nov 19, 2015

gabikliot Nov 20, 2015

dVakulen Nov 20, 2015

gabikliot Nov 20, 2015

dVakulen commented Nov 20, 2015

gabikliot Nov 21, 2015

dVakulen Nov 21, 2015

jason-bragg commented Nov 21, 2015

dVakulen commented Nov 21, 2015

dVakulen commented Nov 21, 2015

dVakulen commented Nov 21, 2015

gabikliot Nov 25, 2015

dVakulen Nov 25, 2015

gabikliot commented Nov 25, 2015

dVakulen commented Nov 25, 2015

gabikliot commented Nov 26, 2015

gabikliot commented Nov 26, 2015

sergeybykov commented Nov 26, 2015

gabikliot commented Nov 26, 2015

dVakulen commented Nov 26, 2015

Coreclr compatibility: Implemented Json.Net fallback serializer #1047

Coreclr compatibility: Implemented Json.Net fallback serializer #1047

Conversation

dVakulen commented Nov 19, 2015

gabikliot Nov 20, 2015

Choose a reason for hiding this comment

dVakulen Nov 20, 2015

Choose a reason for hiding this comment

gabikliot Nov 20, 2015

Choose a reason for hiding this comment

dVakulen commented Nov 20, 2015

gabikliot Nov 21, 2015

Choose a reason for hiding this comment

dVakulen Nov 21, 2015

Choose a reason for hiding this comment

jason-bragg commented Nov 21, 2015

dVakulen commented Nov 21, 2015

dVakulen commented Nov 21, 2015

dVakulen commented Nov 21, 2015

gabikliot Nov 25, 2015

Choose a reason for hiding this comment

dVakulen Nov 25, 2015

Choose a reason for hiding this comment

gabikliot commented Nov 25, 2015

dVakulen commented Nov 25, 2015

gabikliot commented Nov 26, 2015

gabikliot commented Nov 26, 2015

sergeybykov commented Nov 26, 2015

gabikliot commented Nov 26, 2015

dVakulen commented Nov 26, 2015