Cache the delegate for static method group conversions.#58288
Cache the delegate for static method group conversions.#58288AlekseyTs merged 73 commits intodotnet:mainfrom
Conversation
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs
Outdated
Show resolved
Hide resolved
...rs/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.TypeParameterUsageChecker.cs
Outdated
Show resolved
Hide resolved
What aspect do you have in mind? |
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheContainer.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Conversion.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/DelegateCacheRewriter.cs
Outdated
Show resolved
Hide resolved
|
@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. |
|
@AlekseyTs Thanks, I apologize for renaming files in the middle. I think I changed my mind, the original |
|
@jcouv Any additional feedback? |
|
@pawchen Are we good to merge from your point of view? |
|
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! |
|
@pawchen Thank you for the contribution! |
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.
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 <github-actions-autoformatter@xamarin.com>
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
This is the new attempt of #6642:
SynthesizedContainer.static.Considering the complexities currently we have in this PR, we leave the following interesting ideas/improvements for future considerations:
Fixes #5835