-
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
Support For Big Endian Systems #2170
Conversation
35fb50f
to
ab3e871
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2170 +/- ##
==========================================
- Coverage 72.48% 71.30% -1.18%
==========================================
Files 309 306 -3
Lines 61959 61892 -67
==========================================
- Hits 44911 44135 -776
- Misses 17048 17757 +709
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hi, @Wraith2 @David-Engel can you please review this? |
var bytes = new byte[4]; | ||
BinaryPrimitives.WriteInt32LittleEndian(bytes, BitConverter.SingleToInt32Bits(v)); | ||
return bytes; |
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.
Is there a way to do this using the span we already have instead of allocating a new byte[]? If an array is needed it could be stackalloc-ed since we know it is fixed size.
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.
@saitama951 any answer to this query?
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSequentialTextReader.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsValueSetter.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNICommon.cs
Show resolved
Hide resolved
db4fb62
to
4114d8a
Compare
@Wraith2 Thank you for the suggestions, I have made the changes. not sure why the CI is failing, specifically win 2022 one's (Failed to start Sql browser) |
@saitama951 failures are not related to your changes. Somehow starting SqlBrowser task fails on the pipelines. We will look into them. |
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNICommon.cs
Outdated
Show resolved
Hide resolved
...icrosoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.netcore.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsValueSetter.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.NetCoreApp.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Show resolved
Hide resolved
I'm a little concerned about the number of preprocessor regions. If there's any way to extract common patterns into functions so we have fewer ifdefs it'd be nice. Otherwise without the ability to test i'll go with your changes as long as you've got the tests working. Out of interest are you using managed networking? or is there a big endian native sni.dll ? |
94fbadc
to
08d42b1
Compare
I agree with you the if defs are unnecessarily populating the code, I do see the csproj includes a package System.Memory which supports BinaryPrimitives for all the targets but some functions are not supported with netstandard2.0 so I have added a file with the compatibility support, let me know your thoughts on this. |
@DavoudEshtehari |
@DavoudEshtehari |
They're quite noisy. Unless the differences are very large I wouldn't worry about it too much. |
@saitama951 Make sure there is no heavy background process running while your test is running. |
|
||
namespace Microsoft.Data.SqlClient | ||
{ | ||
internal static class BitConverterCompat |
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.
Did we truncate a word here/spelling mistake? Are you trying to say Compatible/Compact? I think it's okay to make it more clear if you need to say Compatible.
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.
@H-Yeo Yes, This was meant to be Compatible (since certain functions of bitconverter were not supported in older frameworks). I will make the necessary change .Thanks
@saitama951 May I ask you to share the perf project and the result from your perf test run? |
@DavoudEshtehari Sorry for the delay in response, I'am running the performance tests present in the SqlClient and was using the ResultComparer tool to compare the results with and without the patch. Initially I was facing a lot of issue in generating a consistent set of data with various runs. As of now I'am able to pertain the consistency and see no reproducible regression, here are the perf results from the various runs. |
In Addition to the files, here is a summary output
Specs:
|
@saitama951 can you address the conflict so we can proceed with the preview release today? |
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.
You have reverted some changes I introduced with #2168 . Note this file is shared between netcore and netfx versions. I highlighted two examples, but there are more lines.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
@saitama951 The last 3 commits look good now. I haven't reviewed the merge commit at all, it will take me some time. By the way I would squash these commits since reviewing them individually is harder. |
27ad06e
to
19d7865
Compare
revert some changes Update TdsParserStateObject.cs change bitconvertercompat to bitconvertercompatible
19d7865
to
499bae3
Compare
The Microsoft SqlClient driver crashes on the s390x. This error stems mostly from the use of BitConverter function used in method definitions across the code and improper byte ordering for big endian systems.
The BitConverter class does conversions according to the endianess of the system it is operating on which causes incorrect reads of little-endian packets sent by the server.