Merge | Netfx SqlTypeWorkarounds

[benrr101]

Description

This PR is less of a merge and more of a total rewrite 😅 . There are a handful of methods that are used in the code to convert between internal representations of a handful of Sql* types (SqlBinary, SqlDecimal, SqlGuid, and SqlMoney). In netcore, we can use built-in methods to get at these internal representations, but netfx will not have access to them. There are equivalent conversion methods available, but they almost always make copies of the data. Thus, we have these methods that directly access the data or internal conversion method for the best performance when reading these types.

The original implementations of these were clunky and confusing, and although it'd be easy to copy-paste them into the common project file, I wanted to make sure we knew what was going on (and whether they were being used). Thus, with a bit of AI magic, I rewrote these conversion methods. Now, I'm sure no one will trust that I did it right - I mean, I don't even trust myself! So, this is my first PR where I'm introducing true unit tests! These were checked against the behavior of the original implementations, so it can be safely assumed that the new implementations do things correctly. I have also added comments to the methods to give a better understanding of how they work, for future people.

Also, say goodbye to dynamic IL method creation!

Issues

#1261

Testing

Added unit tests for the rewritten methods!

[Copilot]

Pull Request Overview

This PR rewrites the SQL type conversion workarounds for .NET Framework (netfx) to improve performance and code clarity while replacing dynamic IL method creation with reflection‐based factories and adding comprehensive unit tests.

• Rewritten conversion methods for SqlBinary, SqlDecimal, SqlGuid, and SqlMoney using direct memory access and unsafe code paths.
• Updated call sites (e.g. in TdsParser, SqlBuffer, and ValueUtilsSmi) to use the new conversion methods.
• Updated project and package references to include System.ValueTuple and removed legacy netfx implementations.

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/UnitTests/Microsoft/Data/SqlTypes/SqlTypeWorkaroundsTests.cs	Added unit tests to validate the new conversion method implementations.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlTypeWorkarounds.cs	Rewritten conversion methods using reflection-based factories and unsafe memory access.
src/Microsoft.Data.SqlClient/SqlBuffer.cs	Replaced deprecated SqlMoneyCtor calls with LongToSqlMoney conversion.
src/Microsoft.Data.SqlClient/Server/ValueUtilsSmi.cs	Updated SqlMoney conversion calls to use new methods.
src/Microsoft.Data.SqlClient/TdsParser.cs	Updated invocations for SqlBinary, SqlGuid, and SqlDecimal conversions; enhanced readability with explicit braces.
csproj and Directory.Packages.props	Added System.ValueTuple dependency and updated related project dependencies.

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlTypeWorkarounds.cs:115

• Consider adding dedicated unit tests to simulate scenarios where reflection fails in CreateSqlToInternalRepresentationFactory and CreateSqlMoneyToLongFactory. This will ensure that the fallback logic is correctly exercised and behaves as expected.

        private static unsafe Func<SqlDecimal, (uint, uint, uint, uint)> CreateSqlToInternalRepresentationFactory()

[Copilot]

Consider refactoring the CreateFactory method to reduce its complexity and improve readability. Splitting its logic or adding more detailed inline comments could ease future maintenance of the unsafe pointer arithmetic and reflection handling.

[David-Engel]

@benrr101 - This is a sensitive hot path. I'd perf check your changes. For reference, check out #1647 (comment). You might be able to reuse the BDN project on that comment.

[edwardneal]

It's good to get the trailing file merged, but I'm not sure switching to using pointers is a real simplification. The dynamic method is well-documented and although it's unusual, it doesn't raise questions about the managed/unmanaged memory layout of SqlDecimal, whether the value needs to be pinned or any performance issues which might arise from copying structs if they're passed by value.

I was planning to use a DynamicMethod when simplifying the UDT serialization so might be slightly biased, but I think keeping the initial approach of emitting a single IL method might be simpler here. The method we emit isn't much more than a chain of field accesses.

[benrr101]

@David-Engel As per your request, here's comparison of perf for old vs new, fast vs fallback. As you can see, almost all of them are faster or are negligibly slower (ie <1%). Though the question is whether this is compliant or not (I'll run a test to see what it says).

[Numbers removed b/c they're out of date now]

---

@edwardneal I'd have to hard disagree on using dynamic methods. Unless there's a huge performance difference between not using them and using them (like 20%+) I think readability and maintainability outweigh it. I think for most cases (at least for our case), engineers are going to be more familiar with pointers than IL opcodes. And in this case, removing the dynamic method actually improves perf by 11% (though some of that might be due to removing checks and some layers of indirection).

Though this did lead me down a trail to learn more about marshaling and blittable types. As near as I can figure, for structs, as long as they aren't defined as using LayoutKind.Automatic there's no difference between unmanaged and managed layouts (and if they are, marshaling will throw). SqlDecimal, doesn't specify a LayoutKind, so I assume it uses 0, which corresponds to Layout.Sequential. Though, I'm curious to learn more about it, if you've got better info for me

[benrr101]

Update, some more information has come to light about considerations for these code paths, and I need to reimplement a couple of them. As such, I'm going to mark this as draft until it's ready. Updated perf numbers will be provided when ready.

And as a heads up to @edwardneal the DynamicMethod will go away, but so will accessing member variables.

[benrr101]

Alrighty, updates are ready for review.

@David-Engel as per your request, here's the perf numbers:

Method	Mean	Error	StdDev	Pct Change
SqlMoney_FromLongFast_Old	8.169 ns	0.0798 ns	0.0708 ns
SqlMoney_FromLongFast_New	7.633 ns	0.0955 ns	0.0893 ns	6.6% faster
SqlMoney_FromLongSlow_Old	8.096 ns	0.1033 ns	0.0966 ns
SqlMoney_FromLongSlow_New	7.634 ns	0.0458 ns	0.0382 ns	5.7% faster
SqlMoney_ToLongFast_Old	7.266 ns	0.0738 ns	0.0691 ns
SqlMoney_ToLongFast_New	7.259 ns	0.0650 ns	0.0608 ns	<1% faster
SqlMoney_ToLongSlow_Old	55.909 ns	1.0257 ns	0.9595 ns
SqlMoney_ToLongSlow_New	7.532 ns	0.0832 ns	0.0778 ns	86% faster
SqlDecimal_WriteTdsValueFast_Old	47.304 ns	0.6128 ns	0.5732 ns
SqlDecimal_WriteTdsValueFast_New	49.798 ns	0.3473 ns	0.3079 ns	5.3% slower
SqlDecimal_WriteTdsValueSlow_Old	50.678 ns	0.7565 ns	0.6706 ns
SqlDecimal_WriteTdsValueSlow_New	50.547 ns	0.6062 ns	0.5374 ns	0.2% faster
SqlBinary_FromByteArrayFast_Old	4,370.409 ns	81.8799 ns	76.5905 ns
SqlBinary_FromByteArrayFast_New	4,276.846 ns	34.5898 ns	28.8840 ns	2.1% faster
SqlBinary_FromByteArraySlow_Old	4,283.448 ns	45.3417 ns	40.1942 ns
SqlBinary_FromByteArraySlow_New	4,273.694 ns	57.0698 ns	47.6559 ns	<1% faster
SqlGuid_FromByteArrayFast_Old	79.946 ns	1.0679 ns	0.9989 ns
SqlGuid_FromByteArrayFast_New	80.508 ns	1.2926 ns	1.2091 ns	<1% slower
SqlGuid_FromByteArraySlow_Old	80.722 ns	1.5184 ns	1.5593 ns
SqlGuid_FromByteArraySlow_New	79.871 ns	1.0893 ns	1.0189 ns	1.1% faster

[edwardneal]

Thanks for checking @benrr101; I've just done the same, and agree - the managed and unmanaged layouts are going to be the same (although that's pretty much academic now.)

The removal of DynamicMethod is probably the reason why SqlDecimal_WriteTdsValueFast has regressed (and it'd be good to see the benchmark's memory allocation figures) but if you're happy with the performance/complexity tradeoff then everything else looks good to me.

[paulmedynski]

even though they

[ErikEJ]

5.3 % slower on decimal? Is that OK - or am I misreading the numbers?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.58%. Comparing base (df35633) to head (a7d4e38).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3448       +/-   ##
===========================================
+ Coverage   63.30%   92.58%   +29.27%     
===========================================
  Files         280        6      -274     
  Lines       62386      310    -62076     
===========================================
- Hits        39493      287    -39206     
+ Misses      22893       23    -22870     

Flag	Coverage Δ
addons	92.58% <ø> (ø)
netcore	?
netfx	?

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[samsharma2700]

Invoking a statically cached method would be faster than redoing Reflection lookup. Something like static readonly MethodInfo s_toSqlInternal = typeof(SqlMoney).GetMethod(...);