Support for non-static serializer #3595

benjaminpetit · 2017-10-24T20:55:27Z

With this PR:

Orleans generated serializers are no longer statics
Developers can implement their own non static serializers. Orleans will create instances with ActivatorUtilities that relies on DI. Static serialization methods are still supported.
Orleans.Core.Abstractions does not depends on System.Reflection.Emit.Lightweight anymore

A shortcut that I took (and that we may want to fix in the future) is that if SerializationManager is instanciated without any IServiceProvider (such as in CodeGenerator), the serializers will not be instanciated. Instead the SerializationManager will only populate the data structure needed for SerializationManager.HasSerializer<T>() (needed for codegen).

jdom · 2017-10-24T21:15:12Z

    private static MemberDeclarationSyntax[] GenerateStaticFields(List<FieldInfoMember> fields)

This and the documentation are no longer true, right? BTW, do we still need these fields? Can't they be used in the constructor as variables and then drop them or are they really needed as fields?

Refers to: src/Orleans.CodeGeneration/SerializerGenerator.cs:367 in f4b5d73. [](commit_id = f4b5d73, deletion_comment = False)

jdom · 2017-10-24T21:21:32Z

src/Orleans.Core/Serialization/SerializationManager.cs

@@ -428,11 +444,12 @@ object DeserializeGeneric(Type expected, IDeserializationContext context)
                }
                else
                {
+                    var target = this.InitializeSerializer(serializerType);


target is missleading, as target usually refers to the target type being serialized, and in this case it is the serializer object itself

jdom · 2017-10-24T21:26:11Z

src/Orleans.Core/Serialization/SerializationManager.cs

+        private object InitializeSerializer(Type serializerType)
+        {
+            if (this.ServiceProvider == null)
+                return null;


Hmmm, this shortcut can be error prone, as there are cases where it is valid to return null at runtime...

I could throw in the GetSerializer/Copier/Deserializer methods if the ServiceProvider wasn't set?

It's fine, since we should be removing this statement entirely in #3669 now that we'll stop using the SM for the codegen path. /cc @ReubenBond

BTW, @ReubenBond, remember to remove this and not allow a null service provider once you rebase your other PR to not use SerializationManager for codegen.

jdom · 2017-10-24T21:26:53Z

src/Orleans.Core/Serialization/SerializationManager.cs

+                return null;
+
+            var constructors = serializerType.GetConstructors();
+            if (constructors == null || constructors.Length < 1)


would default constructors show up in this list even if they aren't explicitly defined in code?

Yes. This will return an empty list only if the constructor is not accessible or the class is static.

jdom · 2017-10-24T21:31:14Z

As I mentioned in person, I'm not that fond of that shortcut, unless the alternative is too costly. I believe we are using SerializationManager mainly as a collection of registered serializers with just a tiny little bit of extra logic in the HasSerializer method. If that's the case, we could remove the dependency on it and just use a HashSet and share that small logic on how to know if there is a serializer defined already.

jdom · 2017-10-24T22:53:47Z

src/Orleans.Core/Serialization/SerializationManager.cs

@@ -575,6 +594,19 @@ internal static void GetSerializationMethods(Type type, out MethodInfo copier, o
            }
        }

+        private Delegate CreateDelegate(MethodInfo methodInfo, Type delegateType, object target)


delegateType could be a generic type parameter, to avoid all the unnecessary casting on the caller side

I previously tried to do that, it wasn't possible to use a generic parameter and cast it in the return statement. It seems that if I use the as operator it will do the trick.

https://stackoverflow.com/questions/35321586/cannot-cast-delegate-to-a-generic-type-t

…y casting

ReubenBond · 2017-10-25T05:09:13Z

src/Orleans.Core/Serialization/SerializationManager.cs

+            if (constructors == null || constructors.Length < 1)
+                return null;
+
+            var serializer = ActivatorUtilities.CreateInstance(this.ServiceProvider, serializerType);


Maybe it's out of scope, but does it make sense to use GetServiceOrCreateInstance here instead since we expect 1 instance of each serializer?

To use that, we would need to register the serializers in the IServiceProvider, correct?

I am not sure we want to register all of them, since you may have A LOT of serializers in your app

No, this means that if it is registered as a service, it will use that, otherwise falls back to CreateInstance.

BTW, are you doing this change or decided not to?

ReubenBond · 2017-10-25T05:14:43Z

src/Orleans.Core/Serialization/SerializationManager.cs

+            if (this.ServiceProvider == null)
+                return null;
+
+            if (!serializerType.GetCustomAttributes(typeof(SerializerAttribute), true).Any())


Worth adding a comment here, like:
// If the type lacks a Serializer attribute then it's a self-serializing type and all serialization methods must be static.

ReubenBond · 2017-10-25T05:15:41Z

src/Orleans.CodeGeneration/SerializerGenerator.cs


                        result.Add(
                            SF.FieldDeclaration(SF.VariableDeclaration(setterType).AddVariables(fieldSetterVariable))
                                .AddModifiers(
-                                    SF.Token(SyntaxKind.PrivateKeyword),
-                                    SF.Token(SyntaxKind.StaticKeyword),
-                                    SF.Token(SyntaxKind.ReadOnlyKeyword)));


not readonly? (here and above)

ReubenBond · 2017-10-25T05:17:12Z

It's worth adding a comment to include what the new generated code looks like, since it's hard to determine without building it.

Left some comments but otherwise LGTM.

rename SerializerGenerator.GenerateStaticFields to GenerateFields

ReubenBond · 2017-11-02T01:19:49Z

src/Orleans.Core/Serialization/SerializationManager.cs

@@ -173,6 +166,15 @@ internal IRuntimeClient RuntimeClient
            RegisterSerializationProviders(serializatonProviderOptionsValue.SerializationProviders);
        }

+        public void SetApplicationPartManager(ApplicationPartManager applicationPartManager)


How does this break the dependency cycle? Is it possible to break it in another way so that SerializationManager doesn't need an initialization step?

To be honest, logically it does make sense that there is an explicit point in time that we populate the serializer feature from the parts and feature providers, instead of relying on luck in when the container happens to resolve the SM

It's deterministic: you cannot access the serialization manager until it's created and the registrations are populated on creation. This change makes it less certain. It's not like SM is populating some state which might already be in use by some other class

The issue is that, if a serializer ask for a SerializationManagerin its constructor, and that we instantiate these serializer in the SerializationManager.... boom.

@benjaminpetit oh right :( Alternatively we could call the method Initialize and store the ApplicationPartManager in the ctor so that it's more obvious that it needs initialization before use.

My preference is for calling this method RegisterSerializers instead and pass the ApplicationPartManager in at this point in time, and is never stored by the SM.

In theory in the future we could call this multiple times (not that we will, nor that we should support it now, but would be applicable logically)

My intention was that registration occurs exactly once, before usage. As a result, not all of the data structures are protected by locks / concurrent collections.

jdom · 2017-11-02T17:50:37Z

src/Orleans.CodeGeneration/SerializerGenerator.cs

+    //    {
+    //            [...]
+    //    }
+    //}


You can use XML comments for these comments

jdom · 2017-11-02T17:55:26Z

src/Orleans.Core/Serialization/SerializationManager.cs

@@ -173,6 +166,15 @@ internal IRuntimeClient RuntimeClient
            RegisterSerializationProviders(serializatonProviderOptionsValue.SerializationProviders);
        }

+        public void SetApplicationPartManager(ApplicationPartManager applicationPartManager)


would RegisterSerializers be a more appropriate name? this is not "setting the app manager for later use", it's commanding the SM to register the serializers (from the app manager) now

…tatic-serializers

jdom · 2017-11-14T21:43:49Z

@dotnet-bot test functional

jdom · 2017-11-15T18:19:39Z

src/Orleans.Core/Core/DefaultClientServices.cs

@@ -34,6 +34,7 @@ public static void AddDefaultServices(IClientBuilder builder, IServiceCollection
            services.TryAddFromExisting<IInternalGrainFactory, GrainFactory>();
            services.TryAddFromExisting<IGrainReferenceConverter, GrainFactory>();
            services.TryAddSingleton<ClientProviderRuntime>();
+            services.TryAddSingleton<IFieldUtils>(new FieldUtils());


any reason why this is passing an instance instead of just the concrete type, as you did in DefaultSiloServices?

Nope, will fix that

benjaminpetit added 2 commits October 24, 2017 13:44

Non static serializers

059ca59

Add tests

f4b5d73

dnfclas added the cla-already-signed label Oct 24, 2017

benjaminpetit requested a review from ReubenBond October 24, 2017 20:55

jdom reviewed Oct 24, 2017

View reviewed changes

Make SerializationManager.CreateDelegate generic to avoid unnecessera…

cfcb4c9

…y casting

ReubenBond reviewed Oct 25, 2017

View reviewed changes

ReubenBond approved these changes Oct 25, 2017

View reviewed changes

Remove useless param in SerializerGenerator.GenerateConstructor and

7efbf36

rename SerializerGenerator.GenerateStaticFields to GenerateFields

jongalloway removed the cla-already-signed label Oct 31, 2017

Fix circular dependency on SerializationManager

2e60c72

ReubenBond reviewed Nov 2, 2017

View reviewed changes

benjaminpetit added 3 commits November 1, 2017 18:33

Make the fields readonly

923da79

Move field declaration as variable in constructor

f68f288

Rename some vars, add some comments

7d6da80

jdom reviewed Nov 2, 2017

View reviewed changes

src/Orleans.CodeGeneration/SerializerGenerator.cs Outdated

// {

// [...]

// }

//}

Copy link

Member

jdom Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use XML comments for these comments

jdom reviewed Nov 2, 2017

View reviewed changes

benjaminpetit added 4 commits November 8, 2017 14:25

Merge branch 'master' of https://github.com/dotnet/orleans into non-s…

ec9b766

…tatic-serializers

Merge branch 'master' of https://github.com/dotnet/orleans into non-s…

8556823

…tatic-serializers

Fix

38d8f8f

Rename method RegisterSerializers

afa8cfd

jdom mentioned this pull request Nov 14, 2017

Re-introduce runtime code generation #3669

Merged

jdom reviewed Nov 15, 2017

View reviewed changes

Fix

bc262d9

jdom approved these changes Nov 15, 2017

View reviewed changes

jdom merged commit 2713356 into dotnet:master Nov 15, 2017

benjaminpetit deleted the non-static-serializers branch March 15, 2018 16:54

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

Support for non-static serializer #3595

Support for non-static serializer #3595

benjaminpetit commented Oct 24, 2017

jdom commented Oct 24, 2017

jdom Oct 24, 2017

jdom Oct 24, 2017

benjaminpetit Oct 24, 2017

jdom Nov 15, 2017

jdom Nov 17, 2017

jdom Oct 24, 2017

benjaminpetit Oct 24, 2017

jdom commented Oct 24, 2017

jdom Oct 24, 2017

benjaminpetit Oct 24, 2017

ReubenBond Oct 25, 2017

benjaminpetit Nov 2, 2017 •

edited

Loading

jdom Nov 2, 2017

jdom Nov 15, 2017

ReubenBond Oct 25, 2017

ReubenBond Oct 25, 2017 •

edited

Loading

ReubenBond commented Oct 25, 2017

ReubenBond Nov 2, 2017

jdom Nov 2, 2017

ReubenBond Nov 2, 2017

benjaminpetit Nov 2, 2017

ReubenBond Nov 2, 2017

jdom Nov 2, 2017

ReubenBond Nov 2, 2017

jdom Nov 2, 2017

jdom Nov 2, 2017

jdom commented Nov 14, 2017

jdom Nov 15, 2017

benjaminpetit Nov 15, 2017

Support for non-static serializer #3595

Support for non-static serializer #3595

Conversation

benjaminpetit commented Oct 24, 2017

jdom commented Oct 24, 2017

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

jdom commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminpetit Nov 2, 2017 • edited Loading

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

ReubenBond Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

ReubenBond commented Oct 25, 2017

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

jdom commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminpetit Nov 2, 2017 •

edited

Loading

ReubenBond Oct 25, 2017 •

edited

Loading