Skip to content
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

Perf | various performance improvements for SqlDataReader, TdsParser, SqlGuid and SqlBinary #2308

Closed
wants to merge 8 commits into from

Conversation

wilbit
Copy link
Contributor

@wilbit wilbit commented Jan 19, 2024

I was working on boxing of SqlBinary particularly, but found more kinda related issues.
The PR fixes:

  • boxing of SqlBinary struct;
  • copying of SqlBinary's internal array of bytes back and forth;
  • creating of empty arrays for null values in SqlDataReader;
  • copying of an array of bytes for SqlGuid.

@Wraith2
Copy link
Contributor

Wraith2 commented Jan 19, 2024

For changes like avoiding the SqlBinary getter consider exceptions. For example, if that code path end up with the wrong data in the buffer, a string or something else, and you request the SqlBinary value currently it's going to throw an InvalidCastException, will your changed code path do the same? Because it's a very old library any externally visible behaviour change is going to break some code somewhere.

Comment on lines 5960 to 5964
#if NET7_0_OR_GREATER
value.SqlBinary = SqlBinary.WrapBytes(unencryptedBytes);
#else
value.SqlBinary = SqlTypeWorkarounds.SqlBinaryCtor(unencryptedBytes, true); // doesn't copy the byte array
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be safe because unencryptedBytes is a parameter so you will need to audit all the callsites to make sure that ownership is always relinquished by the callers.

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: Patch coverage is 64.70588% with 12 lines in your changes missing coverage. Please review.

Project coverage is 56.87%. Comparing base (900d051) to head (f856e2b).
Report is 189 commits behind head on main.

Files with missing lines Patch % Lines
...qlClient/src/Microsoft/Data/SqlClient/SqlBuffer.cs 66.66% 4 Missing ⚠️
...rosoft/Data/SqlTypes/SqlTypeWorkarounds.netcore.cs 0.00% 3 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 66.66% 2 Missing ⚠️
...osoft/Data/SqlClient/Server/ValueUtilsSmi.netfx.cs 0.00% 2 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2308       +/-   ##
===========================================
- Coverage   72.49%   56.87%   -15.63%     
===========================================
  Files         310      304        -6     
  Lines       61868    61557      -311     
===========================================
- Hits        44854    35009     -9845     
- Misses      17014    26548     +9534     
Flag Coverage Δ
addons ∅ <ø> (∅)
netcore 59.62% <60.86%> (-17.08%) ⬇️
netfx 54.35% <69.56%> (-15.59%) ⬇️

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.

@wilbit
Copy link
Contributor Author

wilbit commented Jan 20, 2024

Thanks for the feedback, @Wraith2 , I will consider that.

@cheenamalhotra
Copy link
Member

Closed PR since it's been inactive for more than a month and is not ready to be reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants