Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Runtime Code Generation #528

Merged
merged 1 commit into from
Oct 22, 2015

Conversation

ReubenBond
Copy link
Member

This is a proposed fix for #511 & #510.

Runtime code generation is loaded in the GrainFactory and kept current as new assemblies are loaded. IGrainMethodInvoker implementations are also loaded in GrainTypeManager upon initialization.

Roslyn is used for generating GrainReferences and IGrainMethodInvokers and not for generating GrainReference serializers, Cast methods, or CreateObjectRefernce/DeleteObjectReference methods . Instead, Expression Trees are used in those cases. In the case of GrainReference casters, they of course rely on having a generated GrainReference with the correct marker attribute.

Marker attributes (MethodInvokerAttribute and GrainReferenceAttribute) have been changed to include a GrainType parameter which takes the (possibly generic) type of the grain which they relate to.

This is implemented as an optional plugin for Orleans. If OrleansCodeGenerator.dll is present at runtime, it will be loaded and used. If it is not present, the existing mechanism of compile-time code generation will have to suffice instead.

I have not created a NuGet package, so if this is the approach we are going to take, that will need to be done before the PR is ready to merge.

All criticism is welcome :)

@johnkattenhorn
Copy link

@ReubenBond as always you are the man!

@pherbel
Copy link
Contributor

pherbel commented Jun 17, 2015

Nice stuff @ReubenBond. Have you checked the portability? Would be fine if it is fully portable to .net core.

@ReubenBond
Copy link
Member Author

@pherbel I don't believe so, since there is some potentially forbidden reflection used. How do I test, though? Do I have to merge into the coreclr-compatibility branch?

@sergeybykov
Copy link
Contributor

@pherbel
Copy link
Contributor

pherbel commented Jun 17, 2015

yep Portability Analyzer can help you.
You don't need to merge it to the compatibility branch. There is no new code there.

@gabikliot
Copy link
Contributor

Fantastic @ReubenBond!

@ReubenBond
Copy link
Member Author

There are some portability concerns which I believe we should tackle in a later commit (along with the rest of Orleans). The Portability Analyzer doesn't produce the most obvious results, but I can see that AppDomain.Current.GetAssemblies() will need to be replaced by a different strategy (no problem) and that I need to use TypeInfo instead of Type in several places.

@ReubenBond ReubenBond force-pushed the feature-runtime-codegen branch from 12d0c22 to 062a63e Compare June 18, 2015 10:19
/// </summary>
private readonly ConcurrentDictionary<Type, Delegate> referenceDestoyers =
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure referenceDestoyers isn't needed anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I can tell GrainReference.DeleteObjectReference behaves identically for all GrainReference subclasses.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's probably the case.

@sergeybykov
Copy link
Contributor

Would it be possible to hook this directly into ClientGenerator, so that it would generate compile time code using this new method? I think that would be a great way to test the change for correctness and performance because the exact same code would be generated at compile or run time.

Should we create a feature branch for this?

}
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

This was an oversight and has been remedied.

@ReubenBond ReubenBond force-pushed the feature-runtime-codegen branch from 062a63e to 6f9c2c6 Compare June 19, 2015 02:31
@ReubenBond
Copy link
Member Author

I believe we could hook this directly into ClientGenerator - should I be doing that in this commit or a later one? We can make a branch if you'd like. I'm working on feature-runtime-codegen in my repo.

}
}

if (invoker != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it even possible for invoker to be null here? Shouldn't we throw if for some reason it is? Or is there a legitimate case when it can be null?

@gabikliot
Copy link
Contributor

What is T4? I hope the generated code is compatible. If not, we will have to make it so.

@ReubenBond
Copy link
Member Author

@veikkoeeva the current code is generated by CodeDOM + some string templates (not T4). I'm not sure if CodeDOM is supported in CoreCLR. At the very least there are some changes. I find one instance of CodeDOM in the main CoreCLR github repo.

@gabikliot T4 is a C# templating library from Microsoft. It was the predecessor to Razor.

@veikkoeeva
Copy link
Contributor

Ah, my bad, I should have thought more clearly. I'm corrected. Though it looks either with Roslyn or CodeDOM, when CoreCLR comes, changes are needed.

/// <returns>A completed task.</returns>
public static Task<object> Completed()
{
return Task.FromResult((object)null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use Task.Done from TaskExtensions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not at my PC, but I think Task.Done returns Task, not Task

-----Original Message-----
From: "Dmitry Vakulenko" notifications@github.com
Sent: ‎10/‎22/‎2015 8:56 PM
To: "dotnet/orleans" orleans@noreply.github.com
Cc: "Reuben Bond" reuben.bond@gmail.com
Subject: Re: [orleans] Feature: Runtime Code Generation (#528)

In src/Orleans/Async/TaskUtility.cs:

  •    /// <param name="exception">The exception which the return task faulted with.</param>
    
  •    /// <returns>A faulted task.</returns>
    
  •    public static Task<object> Faulted(Exception exception)
    
  •    {
    
  •        var completion = new TaskCompletionSource<object>();
    
  •        completion.TrySetException(exception);
    
  •        return completion.Task;
    
  •    }
    
  •    /// <summary>
    
  •    /// Returns a completed task.
    
  •    /// </summary>
    
  •    /// <returns>A completed task.</returns>
    
  •    public static Task<object> Completed()
    
  •    {
    
  •        return Task.FromResult((object)null);
    

    Why not use Task.Done from TaskExtensions?

    Reply to this email directly or view it on GitHub.

@gabikliot
Copy link
Contributor

So Roslyn is also not core clr compatible?

@ReubenBond
Copy link
Member Author

I think @veikkoeeva meant, "Both the existing code generator and the one in #528 are incompatible with CoreCLR".

Roslyn is CoreCLR compatible.

@gabikliot
Copy link
Contributor

Ohh, a relief. So there is a way to make it all compatible at the end. Significant effort though.

@sergeybykov
Copy link
Contributor

Don't add src/OrleansCodeGenerator/Orleans.snk, reference src/Orleans.snk instead.

@ReubenBond
Copy link
Member Author

Don't add src/OrleansCodeGenerator/Orleans.snk, reference src/Orleans.snk instead.

Fixed locally.

@ReubenBond ReubenBond force-pushed the feature-runtime-codegen branch from cc2edd1 to 3f1f656 Compare October 22, 2015 22:08
sergeybykov added a commit that referenced this pull request Oct 22, 2015
@sergeybykov sergeybykov merged commit de2902b into dotnet:master Oct 22, 2015
@sergeybykov
Copy link
Contributor

I can't believe I just merged this PR.

Reuben, this is by far the largest and most complicated contribution so far. I can't thank you enough! Sorry, it took that long to get it in.

@ReubenBond
Copy link
Member Author

:D Thank you, Sergey!

@jason-bragg
Copy link
Contributor

Congratz Reuben!
Awesome Contribution!
Thanks for all your effort.

@sergeybykov sergeybykov mentioned this pull request Oct 22, 2015
@gabikliot
Copy link
Contributor

Wow! Awesome! Congratulations and thank you Reuben!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.