-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Optimize generic MethodInfo for Func<TResult> #4588
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
Conversation
@sharwell This seems like a promising change but can we please get before and after performance numbers and speed improvements on typical ML pipelines? |
9974094
to
3042b6f
Compare
@codemzs The only number I will be able to get is the total allocation costs prior to my change in the context of running tests. I'll run a measurement on Microsoft.ML.Tests and show the total allocations within the MarshalInvoke methods and the percentage of those relative to the whole run. |
Due to data collection limitations, I was only able to capture allocations for 11 minutes 20 seconds in the middle of the test run. Total allocations during the recorded time were 143.9GB. A lower bound of allocations directly attributable to the MarshalInvoke pattern (and would be resolved by the pattern seen here) were 44GB. |
b5967e8
to
556baf9
Compare
556baf9
to
d7e1abd
Compare
using Microsoft.ML.Runtime; | ||
|
||
namespace Microsoft.ML.Internal.Utilities | ||
{ |
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.
Any idea why the file names of the newly added files in this directory have an backtick-1 or backtick-2 suffix? Is that an accident or is there a specific reason behind this naming? The change looks good and I am approving it but it would be good if the naming can be fixed.
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.
The file names match the names of the types defined in the files. When generic types are compiled, the compiler adds a `{arity}
suffix to the type name.
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.
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.
The number of generic type parameters.
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.
I'm using the term from here:
https://github.com/dotnet/roslyn/blob/6389e2519f282f7683025761189894e1c894936c/src/Compilers/Core/Portable/Symbols/INamedTypeSymbol.cs#L23-L27
I wanted to link to the language specification but it seems the specification doesn't explicitly give a name to this characteristic of generic types.
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.
Thanks for the additional info. Please feel free to merge.
In reply to: 386566851 [](ancestors = 386566851)
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.
I'll run a build locally to make sure nothing changed in the interim and merge if it passes.
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.
Repeated profiling showed MarshalInvoke as a heavy performance bottleneck. Rather than remove the use of reflection at runtime altogether, this pull request demonstrates a strategy for caching the results of particularly expensive operations. The performance benefits come from two primary changes:
RuntimeReflectionExtensions.GetMethodInfo(Delegate)
is only called once for a given methodMethodInfo.GetGenericMethodDefinition()
is only called once for a given methodIn this change, I am also caching the result of
MethodInfo.MakeGenericMethod
. It might not be necessary to cache this result, but it doesn't seem to hurt either.📝 This pull request is a demonstration of the process required to fully convert the code from the current pattern to the new pattern. I have only converted
MarshalInvoke<TRet>
; each of the others will need to be converted in a similar fashion.