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.

Sign up for GitHub

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

Jump to bottom

Perf | avoid boxing of SqlGuid in SqlBuffer (#2300) #2306

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Perf | avoid boxing of SqlGuid in SqlBuffer (#2300) #2306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5504,7 +5504,7 @@ internal static object GetNullSqlValue(SqlBuffer nullVal, SqlMetaDataPriv md, Sq
break;

case SqlDbType.UniqueIdentifier:
nullVal.SqlGuid = SqlGuid.Null;
nullVal.SetToNullOfType(SqlBuffer.StorageType.SqlGuid);
break;

case SqlDbType.Bit:
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6310,7 +6310,7 @@ private bool TryProcessRow(_SqlMetaDataSet columns, object[] buffer, int[] map,
break;

case SqlDbType.UniqueIdentifier:
nullVal.SqlGuid = SqlGuid.Null;
nullVal.SetToNullOfType(SqlBuffer.StorageType.SqlGuid);
break;

case SqlDbType.Bit:
Expand Down
4 changes: 0 additions & 4 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.netfx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,6 @@ private static void GetNullOutputParameterSmi(SmiMetaData metaData, SqlBuffer ta
// special case SqlBinary, 'cause tds parser never sets SqlBuffer to null, just to empty!
targetBuffer.SqlBinary = SqlBinary.Null;
}
else if (SqlBuffer.StorageType.SqlGuid == stype)
{
targetBuffer.SqlGuid = SqlGuid.Null;
}
else
{
targetBuffer.SetToNullOfType(stype);
Expand Down
9 changes: 6 additions & 3 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBuffer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ internal struct Storage
private bool _isNull;
private StorageType _type;
private Storage _value;
private object _object; // String, SqlBinary, SqlCachedBuffer, SqlGuid, SqlString, SqlXml
private object _object; // String, SqlBinary, SqlCachedBuffer, SqlString, SqlXml

internal SqlBuffer()
{
Expand Down Expand Up @@ -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);
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@wilbit wilbit Jan 20, 2024 •
edited

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)
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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;

Copy link
Contributor Author

@wilbit wilbit Jan 20, 2024 •
edited

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

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor

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 😁

{
_value._guid = value.Value;
}
_type = StorageType.SqlGuid;
_isNull = value.IsNull;
}
Expand Down