Performance | netcore: use new decimal.GetBits overload #3732
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes decimal bit extraction operations by using stack-allocated Span<int> instead of heap-allocated arrays in .NET Core and .NET 5+ environments. The changes reduce heap allocations and improve performance when processing decimal values.
- Replaces
decimal.GetBits()array allocations with stack-allocated spans in .NET targets - Applies the optimization consistently across multiple methods handling decimal/numeric SQL types
- Maintains backward compatibility with .NET Framework using conditional compilation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| TdsParser.cs | Optimizes decimal bit extraction in 8 methods (WriteSqlVariantValue, WriteSqlVariantDataRowValue, WriteSqlMoney, SerializeCurrency, WriteCurrency, AdjustDecimalScale, SerializeDecimal, WriteDecimal) by using stack-allocated spans for .NET targets |
| SqlParameter.cs | Optimizes decimal scale extraction in ValueScaleCore method using stack-allocated span for .NET targets |
|
|
||
| // write the sign (note that COM and SQL are opposite) | ||
| if (0x80000000 == (stateObj._decimalBits[3] & 0x80000000)) | ||
| if ((decimalBits[3] & 0x80000000) == 0x80000000) |
There was a problem hiding this comment.
I would use stateObj._decimalBits here and below instead of decimalBits.
There was a problem hiding this comment.
This would be valid in netfx, but not in netcore - we'll have written the decimal bits to decimalBits, but read values from stateObj._decimalBits.
There was a problem hiding this comment.
Yes, I was assuming we would revert to always setting _decimalBits per my comment above.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3732 +/- ##
==========================================
- Coverage 76.83% 69.44% -7.39%
==========================================
Files 272 266 -6
Lines 45363 45070 -293
==========================================
- Hits 34854 31299 -3555
- Misses 10509 13771 +3262
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Looks great - nice to get rid of _decimalBits.
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
4 participants
Description
.NET 5 introduced an overload of
decimal.GetBitswhich could write data to a stack-allocatedSpan<int>rather than allocating and returning a new array every time. This PR modifies TdsParser and SqlParameter to use this new overload in all cases.We come very close to being able to eliminate the
TdsParserStateObject._decimalBitsfield, but this is used when reading decimals and improving that would require changes to SqlBuffer. I'd like to fix that at the same time as I add test coverage for that scenario.Issues
None.
Testing
Existing tests pass, although I'd appreciate a CI run.