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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize generic MethodInfo for Func<TResult> #4588

Merged
merged 1 commit into from Mar 2, 2020

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Dec 18, 2019

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:

  1. RuntimeReflectionExtensions.GetMethodInfo(Delegate) is only called once for a given method
  2. MethodInfo.GetGenericMethodDefinition() is only called once for a given method

In 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.

@codemzs
Copy link
Member

codemzs commented Dec 18, 2019

@sharwell This seems like a promising change but can we please get before and after performance numbers and speed improvements on typical ML pipelines?

@sharwell
Copy link
Member Author

@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.

@sharwell
Copy link
Member Author

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.

@sharwell sharwell changed the title Optimize generic MethodInfo for Func<TRet> Optimize generic MethodInfo for Func<TResult> Feb 13, 2020
using Microsoft.ML.Runtime;

namespace Microsoft.ML.Internal.Utilities
{
Copy link
Contributor

@harishsk harishsk Mar 2, 2020

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.

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

What does the {arity} represent here?


In reply to: 386548523 [](ancestors = 386548523)

Copy link
Member Author

@sharwell sharwell Mar 2, 2020

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.

Copy link
Member Author

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.

Copy link
Contributor

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)

Copy link
Member Author

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.

Copy link
Contributor

@harishsk harishsk left a comment

Choose a reason for hiding this comment

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

:shipit:

@sharwell sharwell merged commit 2bcb481 into dotnet:master Mar 2, 2020
@sharwell sharwell deleted the faster-calls branch March 2, 2020 19:28
@dotnet dotnet locked as resolved and limited conversation to collaborators Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants