-
Notifications
You must be signed in to change notification settings - Fork 278
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
Remove union overlay design and use reflection in SqlTypeWorkarounds #1647
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1647 +/- ##
==========================================
- Coverage 71.45% 71.35% -0.10%
==========================================
Files 291 293 +2
Lines 61241 61368 +127
==========================================
+ Hits 43757 43792 +35
- Misses 17484 17576 +92
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This will be slower and still relies on internal implementation details of the runtime types. The overlay trick itself isn't a problem it's the fact that it requires private access that is, isn't it? |
The overlay trick is too fragile. Private reflection is ok as long as the target method is documented (see dotnet/docs#28911, dotnet/docs#29900). In the case of the ref emit trick, it takes advantage of the fact that the type's fields are implicitly documented as part of its serialization contract (see |
@David-Engel Calling Sample code: private delegate long SqlMoneyToLongDelegate(ref SqlMoney @this);
var mi = GetFastSqlMoneyToLong();
if (mi is null) { /* fallback */ }
// On Full Framework, invoking the MethodInfo first before wrapping
// a delegate around it will produce better codegen. We don't need
// to inspect the return value; we just need to call the method.
_ = mi.Invoke(new SqlMoney(0), new object[0]);
// Now create the delegate. This is an open delegate, meaning the
// "this" parameter will be provided as arg0 on each call.
var del = (SqlMoneyToLongDelegate)mi.CreateDelegate(typeof(SqlMoneyToLongDelegate), target: null);
// Now we can cache the delegate and invoke it over and over again.
// Note: the first parameter to the delegate is provided *byref*.
SqlMoney money = new SqlMoney(10);
long l1 = del(ref money); // returns 100_000
money = new SqlMoney(20);
long l2 = del(ref money); // returns 200_000 |
@GrabYourPitchforks |
In ns2.0 you need to pull in the packages https://www.nuget.org/packages/System.Reflection.Emit.ILGeneration and https://www.nuget.org/packages/System.Reflection.Emit.Lightweight to get Remember: the only requirement is to avoid using the union overlay trick when running within a Full Framework application. (If you're deployed as a netstandard2.0 package and running within Full Framework, you also have to avoid using union overlays.) If you have some other reliable mechanism to detect that you're not running within a Full Framework application, feel free to use whatever trick you want if it helps simplify your code. A union overlay trick when running within .NET Core would still be fragile and not recommended, but it's not prohibited. |
We should also consider reopening dotnet/runtime#51836 to avoid the need for these types of tricks in the future. Some of the APIs might not be needed - for example, |
I ran some perf tests on the SqlTypesWorkaround methods to see what the difference would be and the results were interesting. Not nearly as consistent as I expected. Original represents using the union overlay design. New represents reflection to call internal methods. Fallback represents all public APIs. Note: "New" on .NET Core and .NET actually results in Fallback public APIs after reflection as the internal methods are not actually present in the final binaries since they get stripped due to not being referenced. Numbers are milliseconds to complete 100,000,000 iterations over the methods. Lower is better. ========================================== .NET Framework
========================================== .NET Core 3.1
========================================== .NET 6
Perf code for reference is here: |
I'm not sure how to interpret the numbers, are they generally acceptible? |
Numbers are in millis. Lower is better. Simple loop of 100,000,000 iterations over each method. Perf isn't my expertise. Feel free to take my zip file and tell me I'm doing it wrong. 😉 |
So SqlBinaryCtor is 42 times slower on net6? that doesn't seem like a performance hit that users will live with. |
I agree. I've been debugging and I found one bug (pushed that change) but the main reason it's so slow on netcore31 and net6 is that CtorHelper.CreateFactory is not finding the constructors against those targets the same way it's finding them for netfx.
The first time through CtorHelper.CreateFactory, it seems to work for the first method it's looking for, but subsequent passes through there result in a null fullCtor. It works for netfx. I'm getting frustrated trying to figure out why. The internal methods are still there on netcoreapp31 and net6 (I realize the net6 one now has a nullable byte[]? parameter but that doesn't explain netcoreapp31). Still working on it. I feel like I need to move things around to see if I'm trying to avoid a .NET bug somewhere. @GrabYourPitchforks Any ideas? |
SqlBinary doesn't have a 2 parameter ctor, it only has two byte[] and boolean. |
@David-Engel Looks like those methods were scrubbed from the .NET Core implementation before it shipped. There's a post-build tool run over .NET Core which looks for uncalled methods and prunes them from the product. Since that ctor was internal and had no visible callers, and since the assembly has no If desired, you can keep the existing overlay trick for netcore. The only place the overlay trick is forbidden is in the netfx implementation, but I think your testing shows that things are within acceptable limits there. For netcore, the overlay would still be very fragile (what happens if |
@JRahnama @DavoudEshtehari @Kaur-Parminder This PR is ready for review now. |
Ran perf numbers with BDN. *New methods try to use reflection and internal methods. *Orig methods use the union overlay hacks. *Slow methods use only public APIs. Since we are now keeping the original design for .NET 3.1 and .NET 6, the relevant numbers are the ones for .NET Framework 4.8. I just kept the others in there for reference. BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19043.1766 (21H1/May2021Update)
Project for reference: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only I'm wondering if there is a way to increase the code coverage on slow paths.
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlTypes/SqlTypeWorkarounds.netfx.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlTypes/SqlTypeWorkarounds.netfx.cs
Show resolved
Hide resolved
…rkarounds Porting dotnet#1647 to 3.1-servicing.
…rkarounds Porting dotnet#1647 to 1.1-servicing
…otnet#1647) * Remove union overlay design and use reflection in SqlTypeWorkarounds * Leave union overlay method intact for netcore # Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj # src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj
…otnet#1647) * Remove union overlay design and use reflection in SqlTypeWorkarounds * Leave union overlay method intact for netcore # Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj # src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj
…otnet#1647) * Remove union overlay design and use reflection in SqlTypeWorkarounds * Leave union overlay method intact for netcore # Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj # src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj # src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlTypeWorkarounds.cs
This replaces the union overlay design against netfx with reflection queries to see if appropriate internal methods exist and utilizes them. If they don't exist, the code falls back to slower public APIs.
The union overlay design is retained against netcore because most of the internal methods used were not referenced by netcore itself and removed by the netcore build process so they don't actually exist. We will push for public APIs in .NET and migrate to them when they are available. Calling the current public APIs in netcore would result in significant performance penalties in this hot path.