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

Research injecting a constructor to avoid reflection in NSObject.AllocateNSObject in the managed static registrar #18357

Closed
Tracked by #18584
rolfbjarne opened this issue May 26, 2023 · 2 comments · Fixed by #18529
Assignees
Labels
enhancement The issue or pull request is an enhancement performance If an issue or pull request is related to performance
Milestone

Comments

@rolfbjarne
Copy link
Member

Problem: we need to set the handle and flags fields before reaching the NSObject ctor.

Solution:

// 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
}

This could potentially make it much faster to create NSObjects in the MSR trampolines.

Ref: dotnet/runtime#86649 (comment)

@rolfbjarne rolfbjarne added enhancement The issue or pull request is an enhancement performance If an issue or pull request is related to performance labels May 26, 2023
@rolfbjarne rolfbjarne added this to the .NET 8 milestone May 26, 2023
@simonrozsival
Copy link
Contributor

Based on the discussion in dotnet/runtime#81741 (comment) I think we should be to implement this without this IL hack using the new UnsafeAccessor attribute. I'm not sure if it would work for private-nested classes but we're already changing visibility of classes in the custom linker steps anyway.

@rolfbjarne I inspected the code we generate and I can't locate where we invoke the constructor via reflection. I believe the call to the .ctor doesn't involve reflection:
image

Are there cases where we would generate different code to call the constructor? I thought that it could be in the case when we create an instance of classes with generic arguments, but those can't be instantiated this way and the UCO throws an exception.

@rolfbjarne
Copy link
Member Author

I inspected the code we generate and I can't locate where we invoke the constructor via reflection.

NSObject.AllocateNSObject calls RuntimeHelpers.GetUninitializedObject (which uses reflection as far as I know):

var obj = (T) RuntimeHelpers.GetUninitializedObject (typeof (T));

@simonrozsival simonrozsival self-assigned this Jun 30, 2023
rolfbjarne pushed a commit that referenced this issue Aug 7, 2023
…18529)

Closes #18357

This PR implements the idea described in the linked issue:

Whenever there's an NSObject constructor that we call from a registrar
callback, we need to create
a separate constructor that will first set the `handle` and `flags`
values of the NSObject before
calling the original constructor. Here's an example of the code we
generate:

```csharp
// The original constructor:
public .ctor (T0 p0, T1 p1, ...) { /* ... */ }

// The generated constructor with pre-initialization: 
public .ctor (T0 p0, T1 p1, ..., IntPtr handle, IManagedRegistrar dummy) {
    this.handle = (NativeHandle)handle;
    this.flags = 2; // Flags.NativeRef == 2
    this..ctor (p0, p1, ...);
}
```

- This code can't be expressed in C# and it can only be expressed
directly in IL.
- The reason we need to do this is because the base NSObject
parameterless constructor
  would allocate a new Objective-C object if `handle` is a zero pointer.
- The `IManagedRegistrar` type is added only to make the constructor
signature unique (IManagedRegistrar is not a public class and customers
can't use it in their apps directly)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue or pull request is an enhancement performance If an issue or pull request is related to performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants