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

Cache the delegate for static method group conversions. #58288

Merged
merged 73 commits into from
Jan 13, 2022

Conversation

pawchen
Copy link
Contributor

@pawchen pawchen commented Dec 12, 2021

This is the new attempt of #6642:

  1. Dropped module scoped containers to reduce complexities, by not dealing with module builders, and utilize the existing SynthesizedContainer.
  2. The target method can be local functions marked as static.
  3. Caching is enabled for C# preview version.
  4. Probably need more tests.

Considering the complexities currently we have in this PR, we leave the following interesting ideas/improvements for future considerations:

  • Improve handling for scenarios where the enclosing method is generic and the target method is a local function
  • Adding type scoped generic containers grouped by the arity
  • Take the branching op out of loops
  • Use fields instead of containers
  • Adding type scoped generic containers grouped by the arity
  • Pulling the cache into the containing type
  • Module scoped cache

Fixes #5835

@pawchen pawchen requested review from a team as code owners December 12, 2021 17:26
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 12, 2021
@AlekseyTs
Copy link
Contributor

@pawchen

Should we look for generic attributes? It's new to me, but probably no need?

What aspect do you have in mind?

@pawchen
Copy link
Contributor Author

pawchen commented Jan 11, 2022

I see that now we have several tests targeting RegularNext version, but we still don't have any test in this file specifically verifying that caching is disabled for Regular10. I think we need at least one test like that.

In reply to: 1007055636

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenMethodGroupConversionCachingTests.cs:5293 in 9ca84f4. [](commit_id = 9ca84f4, deletion_comment = False)

Fair, added in 28ffe27. Please check.

@pawchen
Copy link
Contributor Author

pawchen commented Jan 11, 2022

    G(Target<int>);

It would be good to test that we can share the cache field across multiple functions.

Do we have a test like that?

In reply to: 1007099643

Refers to: src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenMethodGroupConversionCachingTests.cs:4220 in 9ca84f4. [](commit_id = 9ca84f4, deletion_comment = False)

Also added in 28ffe27, please check.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 72)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 72)

@AlekseyTs
Copy link
Contributor

@pawchen I think now it is a good time to rename files and do any other changes you would like to do before we merge this PR. Let us know when you are ready and we squash-merge this PR.

@pawchen
Copy link
Contributor Author

pawchen commented Jan 12, 2022

@AlekseyTs Thanks, I apologize for renaming files in the middle. I think I changed my mind, the original DelegateCacheRewriter is actually good as it has Cache in its name, so I renamed the class instead of the file in 84551f1.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 73)

@AlekseyTs
Copy link
Contributor

@jcouv Any additional feedback?

@AlekseyTs
Copy link
Contributor

@pawchen Are we good to merge from your point of view?

@pawchen
Copy link
Contributor Author

pawchen commented Jan 12, 2022

Yes, we have possible future improvements documented and an issue of Expression Compiler filed. Test cases checked multiple times. Let's get this into preview early, in case we missed any edge cases we could fix them ASAP. Thank you for all the great feedback and guidance!

@AlekseyTs AlekseyTs merged commit ef76bcf into dotnet:main Jan 13, 2022
@ghost ghost added this to the Next milestone Jan 13, 2022
@AlekseyTs
Copy link
Contributor

@pawchen Thank you for the contribution!

@pawchen pawchen deleted the mgcClean branch January 13, 2022 01:03
@jcouv jcouv modified the milestones: Next, 17.2 Feb 1, 2022
@jcouv jcouv added this to the 17.2.P4 milestone Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a cached delegate for method group conversion
3 participants