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

Simplify OptimizeGeneratedCodeHandler to speed up trimming #11477

Open
marek-safar opened this issue May 7, 2021 · 9 comments
Open

Simplify OptimizeGeneratedCodeHandler to speed up trimming #11477

marek-safar opened this issue May 7, 2021 · 9 comments
Labels
dotnet An issue or pull request related to .NET (6) enhancement The issue or pull request is an enhancement
Milestone

Comments

@marek-safar
Copy link
Contributor

Existing OptimizeGeneratedCodeHandler logic duplicates a lot of processing and logic which is done as part of core IL linker steps. The code can be significantly simplified if migrated to use .NET6 features.

A proposal about how this can be done for each of the optimizations

ProcessCalls

EnsureUIThread

Convert field into property returning constant or add feature switch to set the value.

get_Size

Remove completely as it's done by linker by default.

get_IsDirectBinding

Set the constant via https://github.com/mono/linker/blob/main/src/linker/ref/Linker/Annotations.cs#L27-L28 for any override which should return true/false

get_DynamicRegistrationSupported

Remove completely, linker propagates the value already

SetupBlock and SetupBlockUnsafe

This would be much better done with source generator and can probably stay for now as it has nothing to do with linker.

ProcessLoadStaticField

IsARM64CallingConvention

I'm not sure why it's needed as field if we build arm64 specific runtime pack.

Arch

Should be a feature switch with which substitutes field

@spouliot
Copy link
Contributor

spouliot commented May 7, 2021

EnsureUIThread
Convert field into property returning constant

Binary breaking change.

or add feature switch to set the value.

yes, that's was my plan.

@spouliot
Copy link
Contributor

spouliot commented May 7, 2021

get_IsDirectBinding
Set the constant via https://github.com/mono/linker/blob/main/src/linker/ref/Linker/Annotations.cs#L27-L28 for any override which should return true/false

Can you explain how that works ?

IsDirectBinding is not a global constant. The value can be different for each NSObject subclass. In some cases (like if we know there's no subclasses for the type) this can be optimized. Otherwise the check must remain.

@spouliot
Copy link
Contributor

spouliot commented May 7, 2021

get_DynamicRegistrationSupported
Remove completely, linker propagates the value already

This value is true for the original, compile code. However if/when we can remove the dynamic registrar (and drop a lot of code by doing so) then IIRC its updated to return false.

@spouliot
Copy link
Contributor

spouliot commented May 7, 2021

IsARM64CallingConvention
I'm not sure why it's needed as field if we build arm64 specific runtime pack.

The (Xamarin.iOS) managed code has to call different p/invoke signatures based on the ABI.

@spouliot spouliot added dotnet An issue or pull request related to .NET (6) dotnet-pri2 .NET 6: want to have for stable release labels May 7, 2021
@spouliot spouliot added this to the .NET 6 milestone May 7, 2021
@marek-safar
Copy link
Contributor Author

The value can be different for each NSObject subclass. In some cases (like if we know there's no subclasses for the type) this can be optimized.

For those which you know it can be constant, you register the method with Annotations.SetAction (method, MethodAction. ConvertToStub) and set the value via Annotations.SetStubValue (method, 1)

However if/when we can remove the dynamic registrar

It was not clear to me if this should be configurable in net6 if so you keep it as it and add feature switch on top of it

managed code has to call different p/invoke signatures based on the ABI.

Right but it's already constant

return IntPtr.Size == 8 && Arch == Arch.DEVICE;
and if we add feature switch for Arch linker will automatically evaluate the method as constant returning.

@spouliot
Copy link
Contributor

spouliot commented May 7, 2021

For those which you know it can be constant, you register the method with Annotations.SetAction (method, MethodAction. ConvertToStub) and set the value via Annotations.SetStubValue (method, 1)

I might be missing something obvious here but which method are you talking about in your code snippet ?

Example: Let's say that NSData.Foo and NSString.Bar have calls to the IsDirectBinding property.

By analyzing the app we know that

  • NSMutableData is used and that it subclass NSData so we cannot optimize/remove the call to IsDirectBinding;
  • NSString has no subclass (used by the app) so it does not need to call IsDirectBinding

So what's the method to be used ?

  • it can't be NSData.Foo as it can not be optimized (would break the subclasses);
  • NSString.Bar as it can be optimized ? but how does it know it's IsDirectBinding that can be optimized (and other other properties inside the method) ?
  • NSObject.IsDirectBinding property ? it cannot be since optimizing this into NSObject would also apply to NSData ?

@spouliot
Copy link
Contributor

spouliot commented May 7, 2021

or add feature switch to set the value.

yes, that's was my plan.

The easy/initial (not exhaustive) candidates for substitutions I identified are

API Stubbed on
ObjCRuntime.Runtime::RegisterEntryAssembly [1] Device
UIKit.UIButton::VerifyIsUIButton [2] Release
UIKit.UIApplication::EnsureUIThread Release [3]
UIKit.UIApplication::EnsureEventAndDelegateAreNotMismatched Release [4]
UIKit.UIApplication::EnsureDelegateAssignIsNotOverwritingInternalDelegate Release [4]

[1] #10457
[2] #9613
[3] can be overridden (legacy) with optional arguments
[4] were not removed by legacy linker

@marek-safar
Copy link
Contributor Author

You could consider guarding some of them under the existing System.Diagnostics.Debugger.IsSupported feature switch which is unset for Release build.

@marek-safar
Copy link
Contributor Author

I might be missing something obvious here but which method are you talking about in your code snippet ?

This will probably need a different approach than I thought. Based on your description it sounds that IsDirectBinding is another version of typeof (Foo) != GetType () comparison, right? If that's the case I believe the IL Linker will be able to understand that condition soon.

spouliot pushed a commit to spouliot/xamarin-macios that referenced this issue May 12, 2021
…ureUIThread`

Lack of `MONOTOUCH` define in the dotnet csproj [1] made this look for
the AppKit (XM) version.

[1] defining `MONOTOUCH` would have required several more changes.
Simplicity won!

|    Size    | MySingleView.app |
| ----------:|------------------|
|  4,579,985 | legacy           |
| 10,787,541 | dotnet [before](https://gist.github.com/02e072567595088522f376e522e22899) |
| 10,786,829 | dotnet [after](https://gist.github.com/afa5b7d4f9bb281646f050257034be11) |

There are other ways to achieve this with ILLink [2] but again:
Simplicity (by re-using) won!

[2] xamarin#11477

--- a.cs	2021-05-12 14:17:13.000000000 -0400
+++ b.cs	2021-05-12 14:17:17.000000000 -0400
@@ -3764,13 +3764,6 @@
 }
 namespace UIKit
 {
-	public class UIKitThreadAccessException : Exception
-	{
-		public UIKitThreadAccessException()
-			: base("UIKit Consistency error: you are calling a UIKit method that can only be invoked from the UI thread.")
-		{
-		}
-	}
 	[Register("UIApplication", true)]
 	public class UIApplication : UIResponder
 	{
@@ -3824,14 +3817,6 @@
 			UIApplicationMain(args.Length, args, principal, @DeleGate);
 		}

-		public static void EnsureUIThread()
-		{
-			if (CheckForIllegalCrossThreadCalls && mainThread != null && mainThread != Thread.CurrentThread)
-			{
-				throw new UIKitThreadAccessException();
-			}
-		}
-
 		protected internal UIApplication(IntPtr handle)
 			: base(handle)
 		{
@@ -3862,7 +3847,6 @@
 		public UIButton(CGRect frame)
 			: base(NSObjectFlag.Empty)
 		{
-			UIApplication.EnsureUIThread();
 			if (base.IsDirectBinding)
 			{
 				InitializeHandle(Messaging.IntPtr_objc_msgSend_CGRect(base.Handle, Selector.GetHandle("initWithFrame:"), frame), "initWithFrame:");
@@ -3876,7 +3860,6 @@
 		[Export("setTitle:forState:")]
 		public virtual void SetTitle(string title, UIControlState forState)
 		{
-			UIApplication.EnsureUIThread();
 			IntPtr arg = NSString.CreateNative(title);
 			if (base.IsDirectBinding)
 			{
@@ -3948,8 +3931,7 @@
 			[Export("bounds")]
 			get
 			{
-				//Discarded unreachable code: IL_002f
-				UIApplication.EnsureUIThread();
+				//Discarded unreachable code: IL_002b
 				if (base.IsDirectBinding)
 				{
 					_ = 8;
@@ -3965,7 +3947,6 @@
 			[Export("mainScreen")]
 			get
 			{
-				UIApplication.EnsureUIThread();
 				return Runtime.GetNSObject<UIScreen>(Messaging.IntPtr_objc_msgSend(class_ptr, Selector.GetHandle("mainScreen")));
 			}
 		}
@@ -4025,7 +4006,6 @@
 		[Export("addSubview:")]
 		public virtual void AddSubview(UIView view)
 		{
-			UIApplication.EnsureUIThread();
 			IntPtr nonNullHandle = NativeObjectExtensions.GetNonNullHandle(view, "view");
 			if (base.IsDirectBinding)
 			{
@@ -4065,7 +4045,6 @@
 			[Export("view", ArgumentSemantic.Retain)]
 			get
 			{
-				UIApplication.EnsureUIThread();
 				if (base.IsDirectBinding)
 				{
 					return Runtime.GetNSObject<UIView>(Messaging.IntPtr_objc_msgSend(base.Handle, Selector.GetHandle("view")));
@@ -4083,7 +4062,6 @@
 		public UIViewController()
 			: base(NSObjectFlag.Empty)
 		{
-			UIApplication.EnsureUIThread();
 			if (base.IsDirectBinding)
 			{
 				InitializeHandle(Messaging.IntPtr_objc_msgSend(base.Handle, Selector.GetHandle("init")), "init");
@@ -4124,7 +4102,6 @@
 			[Export("setRootViewController:", ArgumentSemantic.Retain)]
 			set
 			{
-				UIApplication.EnsureUIThread();
 				IntPtr arg = NativeObjectExtensions.GetHandle(value);
 				if (base.IsDirectBinding)
 				{
@@ -4146,7 +4123,6 @@
 		public UIWindow(CGRect frame)
 			: base(NSObjectFlag.Empty)
 		{
-			UIApplication.EnsureUIThread();
 			if (base.IsDirectBinding)
 			{
 				InitializeHandle(Messaging.IntPtr_objc_msgSend_CGRect(base.Handle, Selector.GetHandle("initWithFrame:"), frame), "initWithFrame:");
@@ -4160,7 +4136,6 @@
 		[Export("makeKeyAndVisible")]
 		public virtual void MakeKeyAndVisible()
 		{
-			UIApplication.EnsureUIThread();
 			if (base.IsDirectBinding)
 			{
 				Messaging.void_objc_msgSend(base.Handle, Selector.GetHandle("makeKeyAndVisible"));
@@ -4188,7 +4163,6 @@
 		public UIApplicationDelegate()
 			: base(NSObjectFlag.Empty)
 		{
-			UIApplication.EnsureUIThread();
 			base.IsDirectBinding = false;
 			InitializeHandle(Messaging.IntPtr_objc_msgSendSuper(base.SuperHandle, Selector.GetHandle("init")), "init");
 		}
```
spouliot added a commit that referenced this issue May 13, 2021
…ureUIThread` (#11523)

Lack of `MONOTOUCH` define in the dotnet csproj [1] made this look for
the AppKit (XM) version.

[1] defining `MONOTOUCH` would have required several more changes.
Simplicity won!

|    Size    | MySingleView.app |
| ----------:|------------------|
|  4,579,985 | legacy           |
| 10,787,541 | dotnet [before](https://gist.github.com/02e072567595088522f376e522e22899) |
| 10,786,829 | dotnet [after](https://gist.github.com/afa5b7d4f9bb281646f050257034be11) |

There are other ways to achieve this with ILLink [2] but again:
Simplicity (by re-using) won!

[2] #11477

--- a.cs	2021-05-12 14:17:13.000000000 -0400
+++ b.cs	2021-05-12 14:17:17.000000000 -0400
@@ -3764,13 +3764,6 @@
 }
 namespace UIKit
 {
-	public class UIKitThreadAccessException : Exception
-	{
-		public UIKitThreadAccessException()
-			: base("UIKit Consistency error: you are calling a UIKit method that can only be invoked from the UI thread.")
-		{
-		}
-	}
 	[Register("UIApplication", true)]
 	public class UIApplication : UIResponder
 	{
@@ -3824,14 +3817,6 @@
 			UIApplicationMain(args.Length, args, principal, @DeleGate);
 		}

-		public static void EnsureUIThread()
-		{
-			if (CheckForIllegalCrossThreadCalls && mainThread != null && mainThread != Thread.CurrentThread)
-			{
-				throw new UIKitThreadAccessException();
-			}
-		}
-
 		protected internal UIApplication(IntPtr handle)
 			: base(handle)
 		{
@@ -3862,7 +3847,6 @@
 		public UIButton(CGRect frame)
 			: base(NSObjectFlag.Empty)
 		{
-			UIApplication.EnsureUIThread();
 			if (base.IsDirectBinding)
 			{
 				InitializeHandle(Messaging.IntPtr_objc_msgSend_CGRect(base.Handle, Selector.GetHandle("initWithFrame:"), frame), "initWithFrame:");
@@ -3876,7 +3860,6 @@
 		[Export("setTitle:forState:")]
 		public virtual void SetTitle(string title, UIControlState forState)
 		{
-			UIApplication.EnsureUIThread();
 			IntPtr arg = NSString.CreateNative(title);
 			if (base.IsDirectBinding)
 			{
@@ -3948,8 +3931,7 @@
 			[Export("bounds")]
 			get
 			{
-				//Discarded unreachable code: IL_002f
-				UIApplication.EnsureUIThread();
+				//Discarded unreachable code: IL_002b
 				if (base.IsDirectBinding)
 				{
 					_ = 8;
@@ -3965,7 +3947,6 @@
 			[Export("mainScreen")]
 			get
 			{
-				UIApplication.EnsureUIThread();
 				return Runtime.GetNSObject<UIScreen>(Messaging.IntPtr_objc_msgSend(class_ptr, Selector.GetHandle("mainScreen")));
 			}
 		}
@@ -4025,7 +4006,6 @@
 		[Export("addSubview:")]
 		public virtual void AddSubview(UIView view)
 		{
-			UIApplication.EnsureUIThread();
 			IntPtr nonNullHandle = NativeObjectExtensions.GetNonNullHandle(view, "view");
 			if (base.IsDirectBinding)
 			{
@@ -4065,7 +4045,6 @@
 			[Export("view", ArgumentSemantic.Retain)]
 			get
 			{
-				UIApplication.EnsureUIThread();
 				if (base.IsDirectBinding)
 				{
 					return Runtime.GetNSObject<UIView>(Messaging.IntPtr_objc_msgSend(base.Handle, Selector.GetHandle("view")));
@@ -4083,7 +4062,6 @@
 		public UIViewController()
 			: base(NSObjectFlag.Empty)
 		{
-			UIApplication.EnsureUIThread();
 			if (base.IsDirectBinding)
 			{
 				InitializeHandle(Messaging.IntPtr_objc_msgSend(base.Handle, Selector.GetHandle("init")), "init");
@@ -4124,7 +4102,6 @@
 			[Export("setRootViewController:", ArgumentSemantic.Retain)]
 			set
 			{
-				UIApplication.EnsureUIThread();
 				IntPtr arg = NativeObjectExtensions.GetHandle(value);
 				if (base.IsDirectBinding)
 				{
@@ -4146,7 +4123,6 @@
 		public UIWindow(CGRect frame)
 			: base(NSObjectFlag.Empty)
 		{
-			UIApplication.EnsureUIThread();
 			if (base.IsDirectBinding)
 			{
 				InitializeHandle(Messaging.IntPtr_objc_msgSend_CGRect(base.Handle, Selector.GetHandle("initWithFrame:"), frame), "initWithFrame:");
@@ -4160,7 +4136,6 @@
 		[Export("makeKeyAndVisible")]
 		public virtual void MakeKeyAndVisible()
 		{
-			UIApplication.EnsureUIThread();
 			if (base.IsDirectBinding)
 			{
 				Messaging.void_objc_msgSend(base.Handle, Selector.GetHandle("makeKeyAndVisible"));
@@ -4188,7 +4163,6 @@
 		public UIApplicationDelegate()
 			: base(NSObjectFlag.Empty)
 		{
-			UIApplication.EnsureUIThread();
 			base.IsDirectBinding = false;
 			InitializeHandle(Messaging.IntPtr_objc_msgSendSuper(base.SuperHandle, Selector.GetHandle("init")), "init");
 		}
```

Co-authored-by: Sebastien Pouliot <sebastien.pouliot@microsoft.com>
@rolfbjarne rolfbjarne added the enhancement The issue or pull request is an enhancement label Mar 29, 2022
@rolfbjarne rolfbjarne modified the milestones: .NET 6, .NET 7 Mar 29, 2022
@rolfbjarne rolfbjarne removed the dotnet-pri2 .NET 6: want to have for stable release label Mar 29, 2022
@rolfbjarne rolfbjarne modified the milestones: .NET 7, .NET 8 Aug 26, 2022
@rolfbjarne rolfbjarne modified the milestones: .NET 8, .NET 9 Sep 12, 2023
@github-project-automation github-project-automation bot moved this to Technical Debt in .NET 9 Aug 27, 2024
@rolfbjarne rolfbjarne removed this from the .NET 9 milestone Sep 27, 2024
@rolfbjarne rolfbjarne added this to the .NET 10 milestone Sep 27, 2024
@rolfbjarne rolfbjarne removed this from .NET 9 Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dotnet An issue or pull request related to .NET (6) enhancement The issue or pull request is an enhancement
Projects
None yet
Development

No branches or pull requests

3 participants