Re-introduce runtime code generation #3669

ReubenBond · 2017-11-14T10:40:37Z

No description provided.

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

This will conflict with #3595. Which one should go in first? I assume that @benjaminpetit's is ready to be merged

jdom · 2017-11-14T21:47:07Z

test/TesterInternal/RuntimeCodeGenTests.cs

+    public class RuntimeCodeGenTests
+    {
+        [Fact]
+        public void RuntimeCodeGenAddsSupportClasses()


Is it worth adding a functional that starts a silo and verifies the grain and serialization works?

Sure thing, will do

jdom · 2017-11-14T21:50:54Z

Although what you are solving by not requiring a SerializationManager for codegen is what @benjaminpetit wanted to do eventually, but couldn't do yet, so he hacked around it... maybe it's better to merge this one first and have him rebase on this one? I'm fine with whatever you guys decide. Although from what I see about the extension methods discussion this PR might take longer?

jdom · 2017-11-15T19:17:25Z

src/Orleans.CodeGeneration/ApplicationPartManagerCodeGenExtensions.cs

+            tempPartManager.AddFeatureProvider(new BuiltInTypesSerializationFeaturePopulator());
+
+            var codeGenerator = new RoslynCodeGenerator(tempPartManager, new NullLoggerFactory());
+            var generatedAssembly = codeGenerator.GenerateAndLoadForAssemblies(manager.Assemblies);


Does it mean that codegen is run immediately? Shouldn't it wait until a specific point in time just before building the container, so that all the different codegen tasks occur together? Because if I use the WithCodeGeneration extension method multiple times, it will run multiple times. Or did I misunderstand this?

That's correct - code gen will run immediately, but not necessarily for one assembly at a time. The user has a degree of control over it.

In the following code, codegen is executed twice: once for IMyGrain's asm and MyGrain's asm and then again for whatever asms are in the app base dir. The second execution will see the code generated by the first, so it won't double-generate.

.ConfigureApplicationPartManager(parts => { parts.AddApplicationPart(typeof(IMyGrain).Assembly) .AddApplicationPart(typeof(MyGrain).Assembly) .WithCodeGeneration(); parts.AddFromApplicationBaseDirectory().WithCodeGeneration(); });

The above code could execute codegen only once if the methods were all chained together instead of starting again from parts.

Does that seem alright or would you prefer that all actions are queued up and executed only during Build()? That would add some complexity.

I guess it's fine if there is a way to chain everything and then execute once. And if they don't double-generate even when executing twice, then that's harmless anyway.

ReubenBond · 2017-11-21T19:26:24Z

@dotnet-bot test functional

ReubenBond · 2017-11-29T13:18:37Z

@dotnet-bot test functional

jdom · 2017-11-29T17:52:53Z

src/Orleans.CodeGeneration/ApplicationPartManagerCodeGenExtensions.cs

+        /// </summary>
+        /// <param name="manager">The builder.</param>
+        /// <returns>A builder with support parts added.</returns>
+        public static IApplicationPartManagerWithAssemblies WithCodeGeneration(this IApplicationPartManagerWithAssemblies manager)


should the return be void. Otherwise it would be confusing what would happen if you keep chaining, wouldn't it?

you can call WithCodeGeneration().WithReferences(), for example - but we could make it void and force the order. What do you think?

But what would that do? it's not clear to me

It should include those assemblies, generate code for them, and also include their references

jdom · 2017-11-29T17:59:01Z

src/Orleans.Core/Serialization/TypeUtilities.cs

+                   t == typeof(string) ||
+                   t == typeof(DateTime) ||
+                   t == typeof(Decimal) ||
+                   (t.IsArray && t.GetElementType().GetTypeInfo().IsOrleansPrimitive());


You can probably avoid calling GetTypeInfo() since the extension method is now on Type

jdom · 2017-11-29T17:59:58Z

src/Orleans.Core/Serialization/TypeUtilities.cs

                foreach (var parameter in typeInfo.GenericTypeParameters)
                {
                    foreach (var constraint in parameter.GetTypeInfo().GetGenericParameterConstraints())
                    {
-                        if (IsSpecialClass(constraint)) return false;
+                        if (constraint == typeof(Array) || constraint == typeof(Delegate) || constraint == typeof(Enum)) return false;


You dropped ValueType when inlining. Was it by design?

It is: it was supposed to tell us what types cannot be used as generic constraints in C#, but ValueType can (T : struct).

jdom · 2017-11-29T18:00:42Z

src/Orleans.Core.Abstractions/CodeGeneration/KnownAssemblyAttribute.cs

@@ -34,6 +34,7 @@ public KnownAssemblyAttribute(string assemblyName)
        /// serializable.
        /// </summary>
        /// <remarks>This is equivalent to specifying <see cref="KnownTypeAttribute"/> for all types.</remarks>
+        [Obsolete("This property is no longer supported. Serializers will be generated for all accessible types.")]


What's the reason for this change?

It's simpler and less error prone to just consider all types in the few assemblies we're processing

But could be a lot more expensive now that runtime codegen is back, right? I'm not pushing back, just trying to understand

It could be, since we need to perform accessibility checks for more types now. We cache those checks, though, so it shouldn't be an issue.

jdom · 2017-11-29T18:07:04Z

src/Orleans.Core/Core/ClientBuilderExtensions.cs

+        /// <param name="builder">The builder.</param>
+        /// <param name="configure">The configuration delegate.</param>
+        /// <returns>The builder.</returns>
+        public static IClientBuilder ConfigureApplicationPartManager(this IClientBuilder builder, Action<IApplicationPartManager> configure)


How about dropping Manager and just have ConfigureApplicationParts(...)? It is using the manager under the hoods, but the user is configuring which parts the app will use (and of course it's less verbose)

jdom · 2017-11-29T18:09:10Z

src/Orleans.Runtime/Hosting/Generic/SiloHostBuilderExtensions.cs

+        /// <param name="builder">The builder.</param>
+        /// <param name="assembly">The assembly.</param>
+        /// <returns>The builder.</returns>
+        public static ISiloHostBuilder AddApplicationPart(this ISiloHostBuilder builder, Assembly assembly)


I'm on the fence with this one. Do you envision this to be the most common case? Otherwise to make the jump to add references too for example, then it's a lot different, and maybe the best is to get them to use the fluent interface in the first place.

I'm on the fence, too - should we just axe it? Shortcuts like this can be added later if we want, so maybe it's better to be conservative.

My vote is yes: let's axe it for now

Should I wait for this change or you think we should keep it then?

ReubenBond added this to the 2.0.0 milestone Nov 14, 2017

ReubenBond force-pushed the feature-runtime-codegen branch from 9c75068 to da21d5f Compare November 14, 2017 19:06

jdom reviewed Nov 14, 2017

View reviewed changes

jdom mentioned this pull request Nov 15, 2017

Support for non-static serializer #3595

Merged

jdom reviewed Nov 15, 2017

View reviewed changes

ReubenBond force-pushed the feature-runtime-codegen branch from c343a03 to 3827e6c Compare November 17, 2017 03:31

ReubenBond changed the title ~~Expose runtime code generation through ApplicationPartManager~~ [WIP] Expose runtime code generation through ApplicationPartManager Nov 17, 2017

ReubenBond force-pushed the feature-runtime-codegen branch 7 times, most recently from 99681bb to 0ad25cf Compare November 21, 2017 11:52

ReubenBond changed the title ~~[WIP] Expose runtime code generation through ApplicationPartManager~~ Expose runtime code generation through ApplicationPartManager Nov 21, 2017

sergeybykov assigned jdom Nov 28, 2017

ReubenBond force-pushed the feature-runtime-codegen branch 2 times, most recently from 9aa25b9 to 3f1cb95 Compare November 29, 2017 12:18

jdom reviewed Nov 29, 2017

View reviewed changes

ReubenBond added 2 commits December 1, 2017 12:49

Expose runtime code generation through ApplicationPartManager

f7c44a7

Rename ConfigureApplicationPartManager to ConfigureApplicationParts

3f2e254

ReubenBond and others added 4 commits December 1, 2017 12:50

feedback

4c9259e

fixups for runtime codegen

e9f1d13

Use same runtime codegen package name as before

018ab12

Remove builder extensions

96cd529

ReubenBond force-pushed the feature-runtime-codegen branch from c5c498e to 96cd529 Compare December 1, 2017 03:10

jdom changed the title ~~Expose runtime code generation through ApplicationPartManager~~ Re-introduce runtime code generation Dec 1, 2017

jdom merged commit f501f75 into dotnet:master Dec 1, 2017

sergeybykov mentioned this pull request Dec 9, 2017

Serializers are being generated for unneeded types #3771

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

Re-introduce runtime code generation #3669

Re-introduce runtime code generation #3669

ReubenBond commented Nov 14, 2017

jdom commented Nov 14, 2017

jdom Nov 14, 2017

ReubenBond Nov 14, 2017

jdom commented Nov 14, 2017

jdom Nov 15, 2017

ReubenBond Nov 15, 2017 •

edited

Loading

jdom Nov 16, 2017

ReubenBond commented Nov 21, 2017

ReubenBond commented Nov 29, 2017

jdom Nov 29, 2017

ReubenBond Nov 29, 2017

jdom Nov 30, 2017

ReubenBond Nov 30, 2017

jdom Nov 29, 2017

jdom Nov 29, 2017

ReubenBond Nov 29, 2017

jdom Nov 29, 2017

ReubenBond Nov 29, 2017

jdom Nov 30, 2017

ReubenBond Nov 30, 2017

jdom Nov 29, 2017

jdom Nov 29, 2017

ReubenBond Nov 29, 2017

jdom Nov 29, 2017

jdom Nov 30, 2017

Re-introduce runtime code generation #3669

Re-introduce runtime code generation #3669

Conversation

ReubenBond commented Nov 14, 2017

jdom commented Nov 14, 2017

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

ReubenBond Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond commented Nov 21, 2017

ReubenBond commented Nov 29, 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

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

ReubenBond Nov 15, 2017 •

edited

Loading