Merge | SniNativeWrapper Interface

[benrr101]

Description: This is part 2 of 2 for merging SNI native method wrappers. It's a little more involved than before and requires a bit of explanation.

In the netfx version of the SNI native wrapper, the SniNativeWrapper class is truly a wrapper around three native libraries (x86, x64, and arm64). The prior code for this had a switch on each case that would check the architecture of the system and call the appropriate native library. This isn't ideal since it requires a switch statement each time a native SNI call is made. A better approach would be to have the wrapper call into a native library that is picked at static construction of the wrapper.

However, there are limitations to this approach:

1. The only difference between the three native libraries is the name of the [DllImport] dll. However, this must be a compile time constant. This means there must be a class for each of the native libraries.
2. In order to use [DllImport], the method must be extern, which must be static. Interfaces cannot have static methods (at least not prior to net7, iirc), so in order for the library classes to implement an interface, they have to have an instance method that calls into the static, extern method.
3. The easiest way to integrate the netcore implementation into this pattern is to have a implementation of the interface that uses the netcore dll. This introduces a bit of unnecessary indirection, but I think it will be negligible.

Thus, here is the class hierarchy:

• SNINativeMethodWrapper [netfx], SNINativeMethodWrapper.Windows [netcore]
  • ISniNativeWrapper [netfx, netcore]
    • SniNativeMethods [netcore]
    • SniNativeMethodsArm64 [netfx]
    • SniNativeMethodsNotSupported [netfx] - used when architecture is not supported
    • SniNativeMethodsX64 [netfx]
    • SniNativeMethodsX86 [netfx]

Although I can't speak in specifics, when comparing the performance of a switch per call and calling an interface which calls the method, the interface approach is significantly more performant. (sorry @David-Engel )

Testing: For the most part, this just moves code around and doesn't really change the functionality. Everything should still work, but the native SNI CI tests will confirm.

[codecov]

Codecov Report

Attention: Patch coverage is 95.09804% with 15 lines in your changes missing coverage. Please review.

Project coverage is 72.72%. Comparing base (b9877f4) to head (696c949).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...src/Microsoft/Data/SqlClient/LocalDBAPI.Windows.cs	0.00%	4 Missing ⚠️
...Client/src/Interop/Windows/Sni/SniNativeWrapper.cs	98.05%	4 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs	75.00%	3 Missing ⚠️
...t/netfx/src/Microsoft/Data/SqlClient/LocalDBAPI.cs	0.00%	2 Missing ⚠️
...osoft/Data/SqlClient/TdsParserStateObjectNative.cs	95.00%	1 Missing ⚠️
...ft/Data/Sql/SqlDataSourceEnumeratorNativeHelper.cs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3015       +/-   ##
===========================================
- Coverage   92.58%   72.72%   -19.86%     
===========================================
  Files           6      283      +277     
  Lines         310    58975    +58665     
===========================================
+ Hits          287    42892    +42605     
- Misses         23    16083    +16060     

Flag	Coverage Δ
addons	92.58% <ø> (ø)
netcore	75.49% <94.30%> (?)
netfx	71.19% <94.71%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[edwardneal]

I was actually wondering how we were going to approach this merge. I've only got a few comments on the way the PR handles the approach, but are there any wider plans to merge the references to the two native SNI packages?

---

Although I can't speak in specifics, when comparing the performance of a switch per call and calling an interface which calls the method, the interface approach is significantly more performant.

That's true in this case, but the JIT will also make its presence felt. RuntimeInformation.ProcessArchitecture is a constant expression at runtime, so the JIT will see a switch with a constant expression and optimise it away. On .NET Core, you might not actually have a switch expression at all.

[mdaigle]

LGTM