-
Notifications
You must be signed in to change notification settings - Fork 281
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 | avoid boxing of SqlGuid in SqlBuffer (#2300) #2306
Conversation
@dotnet-policy-service agree company="Intrigma" |
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.
As long as the tests pass this looks good to me.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2306 +/- ##
==========================================
- Coverage 72.49% 72.44% -0.06%
==========================================
Files 310 310
Lines 61868 61867 -1
==========================================
- Hits 44854 44821 -33
- Misses 17014 17046 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
My only concern is that SqlBuffer.cs lines 818 and 825-828 aren't exercised by tests and I don't know enough about the changes to know if they are correct or not. Are those lines easily covered by extending one of the existing tests?
Honestly, I have not found any unit tests for I prepare another PR for the similar issue (#2300 (comment)) with boxed |
The logic to set the unmanaged |
If not too difficult to test, yes. Not sure if it's easy (or completely necessary) based on what Wraith2 said.
Thanks for the clarification. I'm fine with the changes, then. |
@@ -815,14 +815,17 @@ internal SqlGuid SqlGuid | |||
} | |||
else if (StorageType.SqlGuid == _type) | |||
{ | |||
return IsNull ? SqlGuid.Null : (SqlGuid)_object; | |||
return IsNull ? SqlGuid.Null : new SqlGuid(_value._guid); |
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.
Have you validated the outcome of this change by comparing the impact of creating a new instance versus unboxing? Overall, I'm not seeing a notable increase in efficiency.
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.
The efficiency isn't gained here, This is just a knockon effect. The efficiency is gained by not assigning a struct SqlNull.Null
to an object which causes an unavoidable box every time it happens.
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.
I just did perf testing and there are some performance and memory degradations brought to this getter by the PR, so, @DavoudEshtehari highlighted a valid point 👍.
I will do more perf testing and try to find a better solution in terms of memory and performance.
} | ||
return (SqlGuid)SqlValue; // anything else we haven't thought of goes through boxing. | ||
} | ||
set | ||
{ | ||
Debug.Assert(IsEmpty, "setting value a second time?"); | ||
_object = value; | ||
if (!value.IsNull) |
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.
If _value
is already set and the new value
is set to null, there might be an inconsistency where _value
still points to a non-null value while _isNull
indicates that it's null. This could lead to confusion and unexpected behavior in your code. It's crucial to ensure that _value
and _isNull
are consistent and accurately represent the state of the variable. IMO, If the intention is to set the variable to null, both _value
and _isNull
should be appropriately updated to reflect this.
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.
_value
is a value type. it is the value it doesn't point to it. If the value is non-empty and then is updated to null it doesn't matter if there is a non-empty value in the variable because it won't be read through any of the code paths.
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.
The class definitely requires its own unit tests.
I have introduced the bug in this PR in SqlBuffer.get_SqlValue
😥:
case StorageType.SqlBinary:
case StorageType.SqlGuid:
return _object;
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.
I don't understand what is the right place to put unit tests.
I thought it is src\Microsoft.Data.SqlClient\tests\FunctionalTests\Microsoft.Data.SqlClient.Tests.csproj
but this project does not target all targets of Microsoft.Data.SqlClient. Particularly, it targets net60
but misses net80
(I'm sure you are aware, but just want to remind that SqlGuid
class has differences between these platforms).
May you point me to the right direction? I want to fix the bug ASAP, @Wraith2 , @David-Engel , @DavoudEshtehari
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.
We don't have a net8 build currently. Write the test and use the correct attribute or preprocessor definition so that when the net8 build does exist (possibly soon-ish) it will be enabled and work.
Functional tests are run without a database connection. Anything that can be checked without running a query can go in there. Manual tests are run with a database connection, anything which can't be in functional tests belongs there.
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.
I vote for a revert given that a release is planned for 24 Jan - just my 2 cents
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.
Here the PR to fix that: #2310
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.
@Wraith2 suggested to revert this PR too. See #2310 (comment)
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.
I just did a PR to revert.
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.
Now we just have to wait for the MS team to work their way through the email notifications, wonder what we've all been doing all weekend, and then approve it 😁
introduced by PR dotnet#2306
This reverts commit 9602271.
fixes #2300