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.

Sign up for GitHub

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

Jump to bottom

Cache the delegate for static method group conversions. #58288

Merged
merged 73 commits into from
Jan 13, 2022
Merged

Cache the delegate for static method group conversions. #58288

merged 73 commits into from
Jan 13, 2022

Conversation

pawchen
Copy link
Contributor

@pawchen pawchen commented Dec 12, 2021 •
edited by jcouv
Loading

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
@runfoapp runfoapp bot mentioned this pull request Dec 13, 2021
Closed
@runfoapp runfoapp bot mentioned this pull request Dec 14, 2021
Closed
@runfoapp runfoapp bot mentioned this pull request Dec 15, 2021
Closed
@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?

jcouv
jcouv approved these changes Jan 11, 2022
View reviewed changes
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)

AlekseyTs
AlekseyTs approved these changes Jan 11, 2022
View reviewed changes
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.

AlekseyTs
AlekseyTs approved these changes Jan 12, 2022
View reviewed changes
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
@pawchen pawchen mentioned this pull request Jan 13, 2022
Closed
@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Jan 15, 2022
Merged
1 task
@jnm2 jnm2 mentioned this pull request Jan 15, 2022
Closed
@RikkiGibson RikkiGibson mentioned this pull request Jan 19, 2022
Closed
@jcouv jcouv modified the milestones: Next, 17.2 Feb 1, 2022
@Tornhoof Tornhoof mentioned this pull request Feb 25, 2022
Merged
@znakeeye znakeeye mentioned this pull request Jun 9, 2022
Closed
@Youssef1313 Youssef1313 mentioned this pull request Jul 21, 2022
Closed
@elachlan elachlan mentioned this pull request Oct 11, 2022
Merged
@jcouv jcouv mentioned this pull request Dec 27, 2022
Open
@tmat tmat mentioned this pull request Mar 9, 2023
Closed
@javiercn javiercn mentioned this pull request Apr 20, 2023
Merged
@jcouv jcouv added this to the 17.2.P4 milestone Sep 17, 2024
mandel-macaque added a commit to dotnet/macios that referenced this pull request Feb 20, 2025
@mandel-macaque
[Bgen] Simplify the BindAs generated code.
a5c3689 
When a property that returns an NSNumber array is marked with the BindAs
attribute, the bgen code generator uses the following pattern:

```csharp
NativeHandle retvalarrtmp;
ret = ((retvalarrtmp = global::ObjCRuntime.Messaging.NativeHandle_objc_msgSend (this.Handle, Selector.GetHandle ("sourceSampleDataTrackIDs"))) == IntPtr.Zero ? null! : (NSArray.ArrayFromHandleFunc <int> (retvalarrtmp, ptr => {
  using (var num = Runtime.GetNSObject<NSNumber> (ptr)!) {
    return (int) num.Int32Value;
  }
}, false)));
```
This code can be simplified to use a static group method. So that it
looks like:
```csharp
NativeHandle retvalarrtmp;
ret = ((retvalarrtmp = global::ObjCRuntime.Messaging.NativeHandle_objc_msgSend (this.Handle, Selector.GetHandle ("sourceSampleDataTrackIDs"))) == IntPtr.Zero ? null! : (NSArray.ArrayFromHandleFunc <int> (retvalarrtmp, NSNumber.ToInt32, false)));
```
A seasoned C# developer would mention that in C# 10 and under using method groups
(specially static ones) alocates more memory than using lambdas (lambdas were cached,
static methods were not, see dotnet/roslyn#5835).
Alas, this is not longer the case after dotnet/roslyn#58288
which means that the memory usage is the same allowing us to clean our generated code.

Using static group methods also opens the door to other possible
improvements mentioned in the roslyn PR.

PS: This is some extra work we are doing to make the generated code
simpler before we move to rgen.
@mandel-macaque mandel-macaque mentioned this pull request Feb 20, 2025
Merged
mandel-macaque added a commit to dotnet/macios that referenced this pull request Feb 20, 2025
@mandel-macaque @rolfbjarne
[Bgen] Simplify the BindAs generated code. (#22216)
3076865 
When a property that returns an NSNumber array is marked with the BindAs
attribute, the bgen code generator uses the following pattern:

```csharp
NativeHandle retvalarrtmp;
ret = ((retvalarrtmp = global::ObjCRuntime.Messaging.NativeHandle_objc_msgSend (this.Handle, Selector.GetHandle ("sourceSampleDataTrackIDs"))) == IntPtr.Zero ? null! : (NSArray.ArrayFromHandleFunc <int> (retvalarrtmp, ptr => {
  using (var num = Runtime.GetNSObject<NSNumber> (ptr)!) {
    return (int) num.Int32Value;
  }
}, false)));
```
This code can be simplified to use a static group method. So that it
looks like:
```csharp
NativeHandle retvalarrtmp;
ret = ((retvalarrtmp = global::ObjCRuntime.Messaging.NativeHandle_objc_msgSend (this.Handle, Selector.GetHandle ("sourceSampleDataTrackIDs"))) == IntPtr.Zero ? null! : (NSArray.ArrayFromHandleFunc <int> (retvalarrtmp, NSNumber.ToInt32, false)));
```
A seasoned C# developer would mention that in C# 10 and under using
method groups (specially static ones) alocates more memory than using
lambdas (lambdas were cached, static methods were not, see
dotnet/roslyn#5835). Alas, this is not longer
the case after dotnet/roslyn#58288 which means
that the memory usage is the same allowing us to clean our generated
code.

Using static group methods also opens the door to other possible
improvements mentioned in the roslyn PR.

PS: This is some extra work we are doing to make the generated code
simpler before we move to rgen.

---------

Co-authored-by: GitHub Actions Autoformatter <[email protected]>
Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcouv jcouv jcouv approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
17.2.P4
Development

Successfully merging this pull request may close these issues.

Use a cached delegate for method group conversion
3 participants
@pawchen @AlekseyTs @jcouv