[CoreFoundation] Fix CFStream to use blittable P/Invokes in .NET 8. #17595
Conversation
This turned out a bit more complex than usual, because the delegate we want to remove from the call to the P/Invoke is a part of the public API. I worked around this by adding new API that uses the new P/Invoke, and having the old API throw an exception until we can remove it, but only in .NET 8, which seems like the earliest we should do this kind of behavioral breaking change.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| [return: MarshalAs (UnmanagedType.I1)] | ||
| static extern bool CFReadStreamSetClient (/* CFReadStreamRef */ IntPtr stream, /* CFOptionFlags */ nint streamEvents, | ||
| /* CFReadStreamClientCallBack */ CFStreamCallback? clientCB, /* CFStreamClientContext* */ IntPtr clientContext); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Instead of InvalidOperationException which will fail at runtime, why not use [Obsolete ("Use the other overload.")] and catch this at compile time?
There was a problem hiding this comment.
I added an Obsolete attribute as well.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Just want to see if I’m following the context of this PR accurately, please let me know if I'm missing anything!
So the process of marshaling focuses on bridging the gap between managed + unmanaged code compatibility by converting the managed code to its corresponding unmanaged representation.
And marshaling occurs on both blittable and non-blittable types, but the key difference is that blittable types have the same representation in memory for both managed + unmanaged code so no further action is required to execute marshaling.
On the contrary, with non-blittable types their representation in memory for managed vs. unmanaged code differs and so they first need to be converted to a form that can be successfully marshaled?
Ok so a few questions:
-
Just curious, is there a specific name for this process for converting non-blittable to blittable types and I’m assuming this conversion handling has a taxing impact on the sdk’s perf?
-
In this PR, the fix to changing CFStream to use blittable PInvokes is essentially a performance enhancement? Because if already blittable, there’s no need to change anything prior to the marshaling process?
And then, PInvokes is one of the solns the CLR provides to enable managed + unmanaged code to work together.
- So, PInvokes are the vehicle through which the actual marshaling is executed and handles the conversion of the managed code to its unmanaged representation?
It's called marshalling: https://learn.microsoft.com/en-us/dotnet/standard/native-interop/type-marshalling
The built-in marshalling is convenient, and generally speaking just replacing the built-in logic with our own code that does the same thing is probably going to take the same amount of time. However, sometimes we can marshal types differently, and that could be faster.
Correct.
Yes, that's one way to put it. Another would be to say that a P/Invoke (Platform Invoke) is just a special type of managed function, which will call a specific unmanaged/native function (as opposed to another managed function). Marshalling is how parameters and return values are converted between their managed representation and the corresponding unmanaged/native representation in this transition- FWIW: the main reason we're trying to stop using the built-in marshalling, is that if we don't use it, all that built-in marshalling code it can be removed from the final app bundle, and hopefully we'll have a nice size decrease. |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻✅ All tests on macOS M1 - Mac Ventura (13.0) passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
🚀 [CI Build] Test results 🚀Test results✅ All tests passed on VSTS: simulator tests. 🎉 All 219 tests passed 🎉 Tests counts✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
|
/sudo backport main |
|
Backport Job to branch main Created! The magic is happening here |
|
Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=7402162 for more details. |
This turned out a bit more complex than usual, because the delegate we want to
remove from the call to the P/Invoke is a part of the public API.
I worked around this by adding new API that uses the new P/Invoke, and having
the old API throw an exception until we can remove it, but only in .NET 8,
which seems like the earliest we should do this kind of behavioral breaking
change.