-
Notifications
You must be signed in to change notification settings - Fork 299
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: Change SqlParameter bool fields to flags #1064
Conversation
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlParameter.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlParameter.cs
Outdated
Show resolved
Hide resolved
SqlParameterFlags.SourceColumnNullMapping | | ||
SqlParameterFlags.IsNull | | ||
SqlParameterFlags.IsNullable | | ||
SqlParameterFlags.IsSqlParameterSqlType | | ||
SqlParameterFlags.CoercedValueIsDataFeed | | ||
SqlParameterFlags.CoercedValueIsSqlType | | ||
SqlParameterFlags.ForceColumnEncryption |
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.
NIT: Could you add this block to the SqlParameterFlags
as a member with a meaningful name?
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.
It doesn't really have a name of it's own, it's just a faster way of copying the individual fields than setting them one at a time. It's only use in this one place so being able to refer to it quickly doesn't seem useful.
Do you have an opinion on whether HasScale and IsDerivedParameterTypeName should be added here?
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.
These two are the SqlParameter properties that describe it and they're part of the SqlParameter object characteristic. So, I don't see any reason why they shouldn't be copied to a cloned object!
Any thoughts @David-Engel
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 Why are all the elements of the SqlParameterFlags enum not included here? I see that HasReceivedMetadata, IsDerivedParameterTypeName, and HasScale are not included. I think that may be what Davoud is really asking. (Or what is special about this subset of items that only they are included?)
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 asked the question earlier, #1064 (comment) and the answer about why is that because they weren't copied in the original. I think they should be, or at least the two I identified. Doing so is a behavioural change though and I avoid those in these merge PR's and call out when I do make them.
If we all agree that they should be included I can add them. You need to be aware that it might change behaviour though.
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.
Ah, I see. HasScale has comments around it indicating it should be ignored
private bool _hasScale; // V1.0 compat, ignore _hasScale
and that code goes way back in netfx. So I don't care about that one.
But IsDerivedParameterTypeName was recently added in #1020, so I'm good with correcting that here and changing the behavior, assuming it was an oversight there.
HasReceivedMetadata is also relatively new, added for AE in SQL 2016. Looking at how it is used, I'm inclined to leave it as-is. (But I'm not saying it's correct or not.)
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 One reason for wrapping these values in SqlParameterFlags
would be drawing special attention to it at the time of adding a new parameter in the future. I believe adding a new member to SqlParameterFlags like Cloneables would clarify it and it doesn't have any downsides.
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.
Ok, done and a comment left to indicate that those were considered so no-one else has to wonder in future.
6a4f845
to
b8e8c13
Compare
Feedback addressed and rebased to master. |
This ports the second part of the changes from In dotnet/corefx#35549 , it replaces many individual boolean fields with bitflags to reduce the size of each instance. I've used masking not Enum.HasFlag because it is portable over netcore and netfx as pointed out by @cmeyertons in #1054
Using ObjectInspector mockups (replaced real ref types with object which are same size), gives a reduction from 152 to 136, a 10% size reduction.