-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
@ReubenBond as always you are the man! |
Nice stuff @ReubenBond. Have you checked the portability? Would be fine if it is fully portable to .net core. |
@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 |
.NET Portability Analyzer can help - https://visualstudiogallery.msdn.microsoft.com/1177943e-cfb7-4822-a8a6-e56c7905292b. |
yep Portability Analyzer can help you. |
Fantastic @ReubenBond! |
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 |
12d0c22
to
062a63e
Compare
/// </summary> | ||
private readonly ConcurrentDictionary<Type, Delegate> referenceDestoyers = |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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.
062a63e
to
6f9c2c6
Compare
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 |
} | ||
} | ||
|
||
if (invoker != null) |
There was a problem hiding this comment.
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?
What is T4? I hope the generated code is compatible. If not, we will have to make it so. |
@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. |
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
So Roslyn is also not core clr compatible? |
I think @veikkoeeva meant, "Both the existing code generator and the one in #528 are incompatible with CoreCLR". Roslyn is CoreCLR compatible. |
Ohh, a relief. So there is a way to make it all compatible at the end. Significant effort though. |
Don't add src/OrleansCodeGenerator/Orleans.snk, reference src/Orleans.snk instead. |
Fixed locally. |
cc2edd1
to
3f1f656
Compare
Feature: Runtime Code Generation
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. |
:D Thank you, Sergey! |
Congratz Reuben! |
Wow! Awesome! Congratulations and thank you Reuben! |
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 inGrainTypeManager
upon initialization.Roslyn is used for generating
GrainReference
s andIGrainMethodInvoker
s and not for generatingGrainReference
serializers,Cast
methods, orCreateObjectRefernce
/DeleteObjectReference
methods . Instead, Expression Trees are used in those cases. In the case ofGrainReference
casters, they of course rely on having a generatedGrainReference
with the correct marker attribute.Marker attributes (
MethodInvokerAttribute
andGrainReferenceAttribute
) have been changed to include aGrainType
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 :)