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

Trimming and extensions compatibility design – discussion #86649

Closed
Tracked by #80905
SamMonoRT opened this issue May 23, 2023 · 27 comments
Closed
Tracked by #80905

Trimming and extensions compatibility design – discussion #86649

SamMonoRT opened this issue May 23, 2023 · 27 comments
Labels
area-NativeAOT-coreclr design-discussion Ongoing discussion about design without consensus os-ios Apple iOS
Milestone

Comments

@SamMonoRT
Copy link
Member

SamMonoRT commented May 23, 2023

Description

Xamarin is heavily dependent on ILLinker and use of custom linker steps in mobile platform builds.

This dependency means that if we want to support building an application targeting iOS-like platforms with NativeAOT, we would have to run ILLinker and ILCompiler (NativeAOT's compiler and trimmer) in cascade during the build.
For such scenarios, the compatibility between the tools arises as an issue, for example:

  • System.Private.CoreLib.dll trimmed by ILLinker cannot be used with ILCompiler, as it requires Array<T> type for compilation, which gets removed in the previous step.
  • Additionally, it can also happen that ILCompiler will trim IL and/or metadata specifically preserved by ILLinker that is required for the application to run.

To use the full potential of both of the tools, we need to make them compatible (to some extent) for targeting mobile devices.

This issue has been opened: to identify how we can reduce the incompatibility gap, to discuss possible approaches, ideas and suggestions on how to get pass this limitation in general with a goal of enabling NativeAOT to work with Xamarin.

Implementation

The implementation of the identified work is being tracked: xamarin/xamarin-macios#18584


/cc: @rolfbjarne @simonrozsival

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 23, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 23, 2023
@ivanpovazan ivanpovazan added design-discussion Ongoing discussion about design without consensus os-ios Apple iOS area-NativeAOT-coreclr and removed untriaged New issue has not been triaged by the area owner needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 23, 2023
@ghost
Copy link

ghost commented May 23, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Xamarin is heavily dependent on ILLinker and use of custom linker steps in mobile platform builds.

This dependency means that if we want to support building an application targeting iOS-like platforms with NativeAOT, we would have to run ILLinker and ILCompiler (NativeAOT's compiler and trimmer) in cascade during the build.
For such scenarios, the compatibility between the tools arises as an issue, for example:

  • System.PrivateCoreLib.dll trimmed by ILLinker cannot be used with ILCompiler, as it requires Array<T> type for compilation, which gets removed in the previous step.
  • Additionally, it can also happen that ILCompiler will trim IL and/or metadata specifically preserved by ILLinker that is required for the application to run.

To use the full potential of both of the tools, we need to make them compatible (to some extent) for targeting mobile devices.

This issue has been opened: to identify how we can reduce the incompatibility gap, to discuss possible approaches, ideas and suggestions on how to get pass this limitation in general with a goal of enabling NativeAOT to work with Xamarin.

Author: SamMonoRT
Assignees: -
Labels:

design-discussion, os-ios, area-NativeAOT-coreclr

Milestone: -

@ivanpovazan ivanpovazan added this to the 8.0.0 milestone May 23, 2023
@agocke
Copy link
Member

agocke commented May 23, 2023

  • @sbomer, who has lots of experience in this area

@jkotas
Copy link
Member

jkotas commented May 23, 2023

System.PrivateCoreLib.dll trimmed by ILLinker cannot be used with ILCompiler, as it requires Array type for compilation, which gets removed in the previous step.

Can this be address by leaving trimming of the runtime libraries to ILCompiler only?

  • Linker w/ extensions would output trimmed versions of the app and Xamarin frameworks.
  • The runtime libraries would be passed unmodified to ILCompiler and ILCompiler would be responsible for trimming them.

Additionally, it can also happen that ILCompiler will trim IL and/or metadata specifically preserved by ILLinker that is required for the application to run.

Can this be addressed by passing a file with reflection roots from Linker to ILCompiler? (The format is TBD.)

@filipnavara
Copy link
Member

filipnavara commented May 23, 2023

Can this be addressed by leaving trimming of the runtime libraries to ILCompiler only?

Yes; that's what I did in the first prototype (the code is now superseded by other iterations in various repository forks).

Can this be addressed by passing a file with reflection roots from Linker to ILCompiler?

Possibly. I had the idea to output rd.xml from the custom linker step that analyzes [Preserve] attributes. It's not very effective to produce a huge .xml file like that and parse it by ILCompiler moment later.

@jkotas
Copy link
Member

jkotas commented May 23, 2023

I had the idea to output rd.xml from the custom linker step that analyzes [Preserve] attributes. It's not very effective to produce a huge .xml file like that and parse it by ILCompiler moment later.

It would be better to produce a managed .dll to pass this info along. For example, it can contain a giant method with a long list of ldtoken + pop pairs to encode what needs to be reflection enabled.

@filipnavara
Copy link
Member

It would be better to produce a managed .dll to pass this info along. For example, it can contain a giant method with a long list of ldtoken + pop pairs to encode what needs to be reflection enabled.

Out of curiosity, when exactly is the method visibility evaluated for ldtoken? Many of the methods marked with the [Preserve] attribute are private constructors.

@jkotas
Copy link
Member

jkotas commented May 23, 2023

ILCompiler does not do any visibility checks. It assumes that it is given valid IL.

@MichalStrehovsky
Copy link
Member

The ldtoken/pop would also be a binary protocol within some well known type/method so even if we were to do visibility/accessibility checks they would be suppressed for the protocol because they wouldn't make sense.

What are the things that are getting trimmed? Is it things that the ObjC interop layer is looking up using reflection at runtime? Could you share pointers to the source code of those places (the spot in iOS interop source code that is failing if ILCompiler trims it)?

@filipnavara
Copy link
Member

filipnavara commented May 23, 2023

What are the things that are getting trimmed? Is it things that the ObjC interop layer is looking up using reflection at runtime? Could you share pointers to the source code of those places (the spot in iOS interop source code that is failing if ILCompiler trims it)?

Yes, interop constructors that are accessed through reflection. One example I hit very early on is the Selector constructor that is access by reflection from one of the Runtime.GetINativeObject overloads.

In this particular case we already discussed some solutions (without modifying ILCompiler behavior; and which would work for user written code using the same pattern):

  • [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicConstructors | DynamicallyAccessedMemberTypes.PublicConstructors)] annotation for the type parameters may result in too many constructors being preserved. Also, my naive attempt to use that resulted in crashing ILLink on assert (not trivial to reduce to simple repro).
  • Generating ldtoken for the method and adjusting the registrar/interop code in a different way to avoid the reflection. This may work if the compiler ignores visibility checks, but the new "Managed Static Registrar" supports all the runtimes, so that's not a perfect option.

@jkotas
Copy link
Member

jkotas commented May 23, 2023

This may work if the compiler ignores visibility checks

You can use IgnoresAccessChecksToAttribute to suppress the visibility checks properly.

@filipnavara
Copy link
Member

You can use IgnoresAccessChecksToAttribute to suppress the visibility checks properly.

I am aware of that but I don't think that's a good idea. The custom ILLink step would rewrite user's assemblies, add that attribute along with the interop code, and it would affect the runtime visibility checks even for the non-interop code. At very least it's an observable change that affects user code.

@jkotas
Copy link
Member

jkotas commented May 23, 2023

I am aware of that but I don't think that's a good idea. The custom ILLink step would rewrite user's assemblies, add that attribute along with the interop code,

It can be its own assembly. (I actually assumed that it would be its own assembly so that it can passed explicitly via command line arg.)

@filipnavara
Copy link
Member

filipnavara commented May 23, 2023

It can be its own assembly. (I actually assumed that it would be its own assembly so that it can passed explicitly via command line arg.)

Ah, sorry, that's a misunderstanding. If we are specifically talking about producing additional input for ILCompiler to preserve methods then it can be separate assembly, and IgnoresAccessChecksToAttribute would not matter since that's totally NativeAOT specific thing.

The idea with ldtoken mentioned above was to produce an input to ILCompiler that would naturally reference the methods in a way that would root them in ILCompiler. For example, instead of calling Runtime.GetINativeObject<Selector>(handle) we could have an GetINativeObject<T>(RuntimeMethodHandle ctorHandle, IntPtr handle) overload where reference to private constructor is loaded with ldtoken and passes as a parameter. In that scenario the caller of Runtime.GetINativeObject and the definition of Selector are often not in the same assembly. Most calls to the Runtime.GetINativeObject overloads are generated as part of custom step in ILLink, and producing a ldtoken there would be easy [if it was not for the access checks in CoreCLR/MonoVM]. Alternatively, we could handle INativeObject similarly to how NSObject is handled and generate all the registrar methods, including callback to call the constructor... That's just a distraction from the original topic though. I wanted to provide the Selector/Runtime.GetINativeObject example. Since we already discussed whether that particular case could be annotated/implemented differently - in a way that ILCompiler would understand without a change - I opted to mention that.

@MichalStrehovsky
Copy link
Member

Looks like this is mostly used in the context of the generics (I don't see anything but dead references to the non-generic methods in the MAUI sample I got). Would it be feasible to change the pattern from:

  • Grab the System.Type that corresponds to the T
  • Reflection locate the constructor with the desired signature and reflection-invoke it

To:

  • Add interface:

    interface INativeObjectCreator<T>
    {
        static abstract T Create(IntPtr ptr);
    }
    
  • Introduce a new GetINativeObject API that is constrained by this interface (or do a breaking change that might be acceptable if INativeObjectCreator would also have default interface method implementation that does the reflection fallback)

  • Implement it on all built in types

  • Simply call the static interface method or take address of it and invoke through a function pointer later

This would be a lot faster and is naturally trim safe. We're using static abstract methods for the COM source generator as well to solve similar problems.

@filipnavara
Copy link
Member

So, conceptually something like this? (deliberately oversimplified constructor lookup and actual parameters)

using System;

Console.WriteLine(CreateINativeObject<NObject>(IntPtr.Zero));
Console.WriteLine(CreateINativeObject<NObject2>(IntPtr.Zero));

static T? CreateINativeObject<T>(IntPtr handle)
    where T : INativeObject<T>
{
    return T.Create(handle);
}

interface INativeObject<TSelf> // : INativeObject
    where TSelf : INativeObject<TSelf>?
{
    static virtual TSelf? Create(IntPtr handle)
    {
        // Default reflection-based implementation?
        var ctor = typeof(TSelf).GetConstructor(new Type[] { typeof(IntPtr) });
        if (ctor != null)
            return (TSelf)ctor.Invoke(new object[] { handle });
        return default;
    }
}

class NObject : INativeObject<NObject>
{
    public NObject(IntPtr handle) { }
}

class NObject2 : INativeObject<NObject2>
{
    private NObject2(IntPtr handle) { }
    
    static NObject2 INativeObject<NObject2>.Create(IntPtr handle) => new NObject2(handle);
}

I suppose that would work. Since INativeObject (non-generic) can be implemented by user code we could have an extra ILLink custom step that generates non-reflection implementation of INativeObject<T> where it's not present. Then the reflection fallback would be redundant.

@MichalStrehovsky
Copy link
Member

Yup, something like that. Softening the blow by just adding the implementations using ILLink custom steps is a nice touch. It would still be nice if this became a future public API (requiring people with their own projections to implement the interfaces so that we can have a custom-step-less future), but maybe it can all be wrapped in custom steps for now.

@simonrozsival
Copy link
Member

Additionally, it can also happen that ILCompiler will trim IL and/or metadata specifically preserved by ILLinker that is required for the application to run.

As far as I understand it, the managed static registrar, solves a lot of problems with dependency analysis for the ILCompiler. We generate UnmanagedCallersOnly methods (with explicit entry points) that provide all the links to the types and methods that aren't called from anywhere else in IL and we only call them from the Objective-C runtime. When we move on to remove the ILLinker and the custom linker steps in some next iteration (.NET 9+) we will need to rethink this part.

Besides the "IntPtr constructors" that Filip mentioned, I can see a few other places where we use reflection in the registrar's code for smart enum converters and for block proxies. I'm not sure if we use it only when we generate the static registrar or if those are used at runtime. I think @rolfbjarne should know this for sure.

Xamarin and MAUI use the [Preserve] throughout their codebases. It's also used commonly in certain libraries and in codebases of Xamarin customers. Based on previous discussions, we don't want to add support for this attribute into ILCompiler. I think we have two options:

  • Pass the information about types and methods marked in custom linker steps to ILCompiler through some file format to preserve backwards compatibility (like the managed dll with ldtoken+pop as discussed earlier in this thread or the various XML files that are already supported).
  • Modify the codebases of our products and remove all usage of [Preserve] (I assume mostly replace it with [DynamicDependency]) and publish some migration guide for our customers to update their apps and libraries.

@ivanpovazan
Copy link
Member

ivanpovazan commented May 24, 2023

An additional consideration:

If we want to get best out of trimming capabilities of both tools (when running them in cascade), we also need to consider the fact that when targeting iOS-like platforms the default trimming behaviour is TrimMode=partial (in ILC achieved through --defaultrooting compiler switch) and that System.AggressiveAttributeTrimming feature is turned on. This means that trimmer(s) can only trim assemblies which are marked with [AssemblyMetadata("IsTrimmable", "True")] but will also trim out such attributes from them.

This prevents ILC to do any additional trimming in this setup because:

  • If ILLinker runs first, it will remove the attribute IsTrimmable from assemblies, which will further prevent ILC of trimming anything additionally as the whole build operates in TrimMode=partial mode.

This is fixable by providing the right set up feature switches at the right moment (just wanted to bring it up in the discussion).

@rolfbjarne
Copy link
Member

rolfbjarne commented May 24, 2023

Out of curiosity, when exactly is the method visibility evaluated for ldtoken? Many of the methods marked with the [Preserve] attribute are private constructors.

I looked at the spec, and it seems no visibility checks are needed for ldtoken instructions to be verifiable.

Yes, interop constructors that are accessed through reflection. One example I hit very early on is the Selector constructor that is access by reflection from one of the Runtime.GetINativeObject overloads.

In this particular case we already discussed some solutions (without modifying ILCompiler behavior; and which would work for user written code using the same pattern):

  • [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicConstructors | DynamicallyAccessedMemberTypes.PublicConstructors)] annotation for the type parameters may result in too many constructors being preserved. Also, my naive attempt to use that resulted in crashing ILLink on assert (not trivial to reduce to simple repro).
  • Generating ldtoken for the method and adjusting the registrar/interop code in a different way to avoid the reflection. This may work if the compiler ignores visibility checks, but the new "Managed Static Registrar" supports all the runtimes, so that's not a perfect option.

Given that we're already modifying IL during the build, there's a simpler solution:

  1. Change the visibility of these constructors to be public.
  2. Add a Runtime.TryGetINativeObject method that returns if no existing object was found, and then just change the calls to Runtime.GetINativeObject to call Runtime.TryGetINativeObject instead and if that fails, call the constructor directly.

Now the proposal to use an interface with a static abstract Create method is also interesting, if only to avoid the reflection code path on other runtimes, so I'll have a look at that.

@agocke
Copy link
Member

agocke commented May 24, 2023

Xamarin and MAUI use the [Preserve] throughout their codebases. It's also used commonly in certain libraries and in codebases of Xamarin customers. Based on previous discussions, we don't want to add support for this attribute into ILCompiler. I think we have two options:

  • Pass the information about types and methods marked in custom linker steps to ILCompiler through some file format to preserve backwards compatibility (like the managed dll with ldtoken+pop as discussed earlier in this thread or the various XML files that are already supported).

The problem with the [Preserve] attribute is that any implementation (in the linker or anywhere else) needs to scan all the IL in the app to look for it. One of the reasons the ILC compiler is so fast is that it doesn't need to scan all the IL. I would recommend designing with the intent of eventually getting rid of [Preserve], since it sounds like publish speed is a key metric for your customers.

@MichalStrehovsky
Copy link
Member

Besides the "IntPtr constructors" that Filip mentioned, I can see a few other places where we use reflection in the registrar's code for smart enum converters and for block proxies.

Could you point me to those? I'd really like us to get into a state where discussing additional means of rooting things is the last resort. I have a couple reasons:

  1. Reliability: The way we reimagined trimming after .NET 5 is that we rely on static analysis to flag trimming unsafe patterns. We converted most of the reflection use within the framework to use trimming safe patterns. In very few places we use DynamicDependency and trim analysis warning suppressions. The places where we do this have been sources of bugs. It is the last ditch resort that we don't do lightly.
  2. Size: In NativeAOT we can trim a lot more if things are not considered targets of reflection. For example in the GetINativeObject case, if we reflection-root the constructor, we need to keep: standalone method body, unwinding information for the method body, GC information for it, name of the method, types of parameters and their names, any custom attributes on the constructor or its parameters (nullable annotations, etc). If we instead access the constructor through the static abstract method like I proposed, there's a good chance this gets resolved at compile time and inlined into codegen. If the constructor was simple, it may literally turn into a single instruction within a method body that is calling it. The point is that the size overhead can be dozens, even hundreds, of times smaller without rooting things as reflectable. This is where NativeAOT has advantage over how AOT is currently done in Mono. Reflection rooting the method removes any gains we'd have had from it. Similar approach is being investigated in Mono AOT (Optimizing generics by using ILCompiler dependency graph and profile guided optimization in Mono AOT #80941) but if appmodels take dependency on reflecting on stuff, we'll get a lot less benefit than we could have.
  3. Startup: if we spend a bunch of time at startup reflecting on things that could have been resolved at compile time, it's a wasted time. This time is equally wasted on Mono as it is wasted with NativeAOT. The reflection stacks likely have similar perf characteristics. There's no advantage to be had for NativeAOT. The basis of the experiment is that with NativeAOT we might be able to get better startup time. But if we spend startup in code that cannot possibly be faster, it diminishes the returns.

I think spending some time on investigating the ways to get rid of the reflection could provide more visible immediate improvements even for customers that don't try the NativeAOT experiment.

and that System.AggressiveAttributeTrimming feature is turned on

My suggestion is to condition _AggressiveAttributeTrimming on PublishAot not being turned on in the SDK. Savings from aggressive attribute trimming are peanuts on NativeAOT - I've not seen this save more than 0.5%. Alternative we could fiddle with removing it from the feature switches when ILLinker sees it and adding it back for Native AOT but I don't know if it's really worth the effort.

@rolfbjarne
Copy link
Member

rolfbjarne commented May 25, 2023

Besides the "IntPtr constructors" that Filip mentioned, I can see a few other places where we use reflection in the registrar's code for smart enum converters and for block proxies.

These two cases don't use reflection anymore when using the Managed Static Registrar (i.e. for NativeAOT).

Could you point me to those? I'd really like us to get into a state where discussing additional means of rooting things is the last resort.

We use reflection in the following scenarios:

When instantiating any custom managed subclasses of NSObject from Objective-C.

Say we have the following managed class:

class MyObject : NSObject {
    [Export ("initWithSomething:")]
    public MyObject (int something) {}
}

In this case we do something like:

partial class MyObject {
    class __RegistrarHelper_ {
        [UnmanagedCallersOnly ("__MyObject_initWithSomething__")]
        static void DoSomething (IntPtr handle, IntPtr selector, int arg1)
        {
            var obj = RuntimeHelpers.GetUninitializedObject (type);
            var ctorHandle = ldtoken (MyObject..ctor);
            var ctor = MethodBase.GetMethodFromHandle (ctorHandle);
            obj.Handle = handle; // The Handle field needs to be set before calling the constructor
            MyObject..ctor (arg); // IL: callvirt MyObject..ctor
        }
    }
}

I don't think we have a way around using RuntimeHelpers.GetUninitializedObject, because we need to set the Handle field before NSObject's constructor is called (NSObject's constructor needs to know whether to create a native instance, or if a native instance already exists and we're just creating a managed wrapper instance around it).

One idea might be to introduce a generic RuntimeHelpers.GetUninitializedObject<T> () method, but that would require a change in the BCL.

When calling managed methods on a generic type from Objective-C.

Say we have the following managed class:

class MyObject<T> : NSObject where T: NSObject {
    [Export ("doSomething:")]
    public void DoSomething () {}
}

the generated UnmanagedCallersOnly method will be something like:

partial class MyObject<T> {
    class __RegistrarHelper_ {
        [UnmanagedCallersOnly ("__MyObject_DoSomething__")]
        static void DoSomething (IntPtr handle, IntPtr selector)
        {
            var obj = Runtime.GetNSObject (handle);
            var openTypeHandle = ldtoken (MyObject<T>);
            var openMethodHandle = ldtoken (MyObject<T>.DoSomething);
            // Runtime.FindClosedMethod:
            // https://github.com/xamarin/xamarin-macios/blob/3dd412daa6b5522029014b0be298f27d751c5f45/src/ObjCRuntime/Runtime.cs#L2182-L2189
            MethodInfo closedMethod = Runtime.FindClosedMethod (obj, openTypeHandle, openMethodHandle);
            closedMethod.Invoke (new object [] {});
        }
    }
}

I don't think there's a way around using reflection here, since the UCO entry point doesn't know the closed generic type/method, and we need to figure out in order to call the target method.

When instantiating a managed NSObject instance (or subclass) for an existing Objective-C object

Say we have the following managed class:

class MyObject : NSObject {
    [Export ("doSomething:")]
    public static void DoSomething (NSString something) {}
}

class NSString : NSObject {
    protected NSString (IntPtr handle) : base (handle) {}
}

In this case we do something like:

partial class MyObject {
    class __RegistrarHelper_ {
        [UnmanagedCallersOnly ("__MyObject_DoSomething__")]
        static void DoSomething (IntPtr handle, IntPtr selector, IntPtr arg1)
        {
             // Runtime.GetNSObject:
             // https://github.com/xamarin/xamarin-macios/blob/3dd412daa6b5522029014b0be298f27d751c5f45/src/ObjCRuntime/Runtime.cs#L1516-L1519
             // basically something like this:
             //     if we already have a managed instance for the handle 'handle', return that
             //     otherwise find the constructor that takes an IntPtr (using reflection), and call that constructor
            var arg1Obj = Runtime.GetNSObject ();
                
            MyObject.DoSomething (arg1Obj);
        }
    }
}

This is in some ways very similar to Filip's INativeObject/Selector case from above, except a bit more complex, because Runtime.GetNSObject will look up the actual runtime type of the Objective-C handle, find the closest matching C# type, and create an instance of that type. This might very well be a subclass of the type the caller expects (and this is why we can't just do a new NSString (handle) in the generated code).

Potential solution: we already generate a table to map Objective-C class -> managed type (which we use to find the closest matching C# type for a given Objective-C classr), we could add the token for the IntPtr ctor to table, so that when we find a managed type, we'll find the ctor to call too (or maybe even generate code to call the ctor directly, because calling a method given a token in C# is rather obnoxious)

3. Startup: if we spend a bunch of time at startup reflecting on things that could have been resolved at compile time, it's a wasted time. This time is equally wasted on Mono as it is wasted with NativeAOT. The reflection stacks likely have similar perf characteristics. There's no advantage to be had for NativeAOT. The basis of the experiment is that with NativeAOT we might be able to get better startup time. But if we spend startup in code that cannot possibly be faster, it diminishes the returns.

Agreed - and that's really what the static registrar does, computes as much as possible during build, so that we can do as little as possible during app execution. Note that we haven't been able to spend much time in recent years to take advantage of improvements in the BCL and the runtimes, so there are certainly many locations where we can do better.

@simonrozsival
Copy link
Member

I don't think we have a way around using RuntimeHelpers.GetUninitializedObject, because we need to set the Handle field before NSObject's constructor is called (NSObject's constructor needs to know whether to create a native instance, or if a native instance already exists and we're just creating a managed wrapper instance around it).

Could we move the AllocIfNeeded from the constructor and call it in the UCO itself? We would then always create the .NET object as a wrapper for an existing handle. Could we then set the handle after we call the constructor or is there a different use case where the constructor needs to have access to the handle? Or maybe we could somehow enforce (through IL weaving I guess) that all constructors that are called from UCOs take the handle as an argument and pass it to the base class constructor? Is that too clunky or would it be a major breaking change?

I don't think there's a way around using reflection here, since the UCO entry point doesn't know the closed generic type/method, and we need to figure out in order to call the target method.

Does the Objective-C code that calls the UCO know what the generic types are? Could it pass the "type ids" to the UCO as an argument and then use some generated lookup table to find the method we should call?

@rolfbjarne
Copy link
Member

Could we move the AllocIfNeeded from the constructor and call it in the UCO itself? We would then always create the .NET object as a wrapper for an existing handle. Could we then set the handle after we call the constructor or is there a different use case where the constructor needs to have access to the handle?

A managed object can be created in two ways:

  1. Directly from managed code:
new NSString ("whatever);

in this case we'll create a native NSString instance in the NSObject constructor (in AllocIfNeeded).

Note that in this case there's no UCO method involved.

  1. As a wrapper for an existing native NSString instance.

In this case we must not create anything in AllocIfNeeded - so there's nothing that can be hoisted into the UCO method.


There is one rather ugly solution: inject a new constructor into the type we need to construct, and chained through a new constructor in all base classes until we reach a special constructor in NSObject.

So something like this:

class UIView : UIResponder {
    public UIView (IntPtr handle, ... <other arguments to ensure this ctor doesn't have the same signature as an existing ctor>)
	: base (handle, ... <other arguments>) {}
}
class UIResponder : NSObject {
    public UIResponder (IntPtr handle, ... <other arguments to ensure this ctor doesn't have the same signature as an existing ctor>)
	: base (handle, ... <other arguments>) {}
}
class NSObject {
    public NSObject (IntPtr handle, ... <other arguments to ensure this ctor doesn't have the same signature as an existing ctor>)
    {
        // Don't call AllocIfNeeded here, because we know we already have a native object.
    }
}

I don't think there's a way around using reflection here, since the UCO entry point doesn't know the closed generic type/method, and we need to figure out in order to call the target method.

Does the Objective-C code that calls the UCO know what the generic types are? Could it pass the "type ids" to the UCO as an argument and then use some generated lookup table to find the method we should call?

No, the only thing that knows the closed generic type is the managed instance the method is called on.

@MichalStrehovsky
Copy link
Member

When instantiating any custom managed subclasses of NSObject from Objective-C.

There is one rather ugly solution: inject a new constructor into the type we need to construct, and chained through a new constructor in all base classes until we reach a special constructor in NSObject.

Would this be fixable by just injecting one constructor? I don't think we need to chain it - we'd basically inject:

// Handle32 is an enum - its only purpose is to be unique
// Alternatively, we could use IntPtr and put a custom modifier on the parameter
// The custom modifier also guarantees a unique signature that cannot possibly conflict with user constructors
.method public hidebysig specialname rtspecialname 
        instance void  .ctor(valuetype Handle32 handle) cil managed
{
  // Store the handle first
  // This should be legal. It's a violation of CLS rules but not a problem for correctness or even verifiability
  ldarg.1
  ldarg.0
  stfld NSObject.Handle
  // Chain to the constructor we wanted to call
  // Load any parameter if there's any
  ldarg.0
  call       instance void Blah::.ctor()
  ret
}

When calling managed methods on a generic type from Objective-C.

I don't have any good idea on what to do there. But this is not fundamentally trimming unsafe - we have a handle for the open method, we have an instance of the object, and we can therefore find the method. This should not be generating any trimming warnings. The implementation of FindClosedMethod right above the one you linked could be updated to use Type.GetMemberWithSameMetadataDefinitionAs instead of iterating over all methods and comparing tokens - it's faster.

Even though I don't have trimming/preservation concerns around this, this pattern will not win any perf prizes. Cc @jkoritzinsky @AaronRobinsonMSFT for ideas. I think in general we solve this by banning generics in other interops.

When instantiating a managed NSObject instance (or subclass) for an existing Objective-C object

Having some form of lookup table for this would help, like you write. We could also generate "creator" methods that just call the right constructor based on a series of ifs. I don't know which one would be better.

@rolfbjarne
Copy link
Member

Would this be fixable by just injecting one constructor? I don't think we need to chain it - we'd basically inject:

That's an interesting idea, it's curious what we can actually do in IL / when not limited to C#...

Even though I don't have trimming/preservation concerns around this, this pattern will not win any perf prizes. Cc @jkoritzinsky @AaronRobinsonMSFT for ideas. I think in general we solve this by banning generics in other interops.

@simonrozsival had an interesting idea here, basically injecting an interface and using the vtable to indirectly lookup the closed generic type: https://dotnetfiddle.net/orpEt8

In any case we've documented this as being slow, and customers can easily make it faster if they wish, so I'm not all that concerned about speed. The main benefit would probably be to be able to trim away a lot of reflection code in the BCL.

dalexsoto added a commit to xamarin/xamarin-macios that referenced this issue Jul 17, 2023
In the current setup with NativeAOT, during app build we run both ILLink
and ILCompiler in cascade.
When `_AggressiveAttributeTrimming` feature switch is set to `true`
ILLink removes `IsTrimmable` attribute from assemblies, which means that
when we are also running in `TrimMode=partial` (which translates to
`--defaultrooting` ILC command-line arguments) ILC trimming is disabled
completely.

This PR disables `_AggressiveAttributeTrimming` in the first pass ie
during trimming by ILLink and enables it in the second trimming pass
performed by ILCompiler.

Additionally, to workaround ILCompiler incompatibility with
`Microsoft.iOS` (and friends) this platform assembly is explicitly
rooted when passed to ILCompiler for trimming (this will be fixed once
dotnet/runtime#86649 is resolved).

Estimated savings: This change reduces the size of the application
bundle by `0,58Mb` (or ~4,3% compared to the baseline)

| MAUI ios app | Base | This PR | diff (%) |
|--------------|-----------|-----------|----------|
| SOD (Mb)     | 41,93 | 40,5      | -3,4% |
| .ipa (Mb)    | 13,43  | 12,85    | -4,3% |


Fixes: #18479

---------

Co-authored-by: Alex Soto <[email protected]>
@MichalStrehovsky
Copy link
Member

I think we resolved all of the problematic cases and there's now individual issues tracking actual work. I'm going to close this but please reopen if something still needs attention in this issue.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr design-discussion Ongoing discussion about design without consensus os-ios Apple iOS
Projects
Archived in project
Development

No branches or pull requests

8 participants