Skip to content

Perf: Stop creating parameter prefixed names#1829

Merged
JRahnama merged 1 commit into
dotnet:mainfrom
Wraith2:perf-paramnameappend
Apr 10, 2023
Merged

Perf: Stop creating parameter prefixed names#1829
JRahnama merged 1 commit into
dotnet:mainfrom
Wraith2:perf-paramnameappend

Conversation

@Wraith2

@Wraith2 Wraith2 commented Nov 6, 2022

Copy link
Copy Markdown
Contributor

A fairly straight forward set of changes to avoid creating @ prefixed parameter names unless we need the string, for example in an error message. Most of the time we can either do a comparison that takes into account the prefixed and unprefixed possibilities or we can append the @ and then the parameter name if we just check what we already have.

Note that this does not change the parameter name that the user provides and will have no visible change as far as users are concerned. We're just being a little more careful about generating gen0 strings that are going to be thrown away.

@codecov

codecov Bot commented Nov 6, 2022

Copy link
Copy Markdown

Codecov Report

Base: 71.41% // Head: 71.39% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (7ae039b) compared to base (146c34e).
Patch coverage: 92.72% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1829      +/-   ##
==========================================
- Coverage   71.41%   71.39%   -0.02%     
==========================================
  Files         290      291       +1     
  Lines       61280    61362      +82     
==========================================
+ Hits        43761    43811      +50     
- Misses      17519    17551      +32     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 74.93% <92.42%> (-0.07%) ⬇️
netfx 69.38% <92.53%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs 83.50% <90.00%> (+0.07%) ⬆️
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs 73.59% <90.32%> (+0.43%) ⬆️
...lient/src/Microsoft/Data/SqlClient/SqlParameter.cs 75.70% <90.47%> (+0.30%) ⬆️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 73.41% <100.00%> (+0.04%) ⬆️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 70.95% <100.00%> (-0.10%) ⬇️
.../Microsoft/Data/SqlClient/SqlQueryMetadataCache.cs 62.06% <100.00%> (ø)
...core/src/Microsoft/Data/SqlClient/SNI/SNIHandle.cs 21.42% <0.00%> (-20.68%) ⬇️
...re/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs 68.85% <0.00%> (-9.84%) ⬇️
...icrosoft/Data/SqlClient/LocalAppContextSwitches.cs 56.25% <0.00%> (-8.75%) ⬇️
.../src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs 87.89% <0.00%> (-4.49%) ⬇️
... and 13 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.


// Find the return value parameter (if any).
SqlParameter returnValueParameter = null;
foreach (SqlParameter parameter in parameters)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not keep the full name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because a variable with that name is created and used later in this scope. I don't want to explicitly re-use the same variable because it can leave opportunity for errors to creep in with casual refactorings in future. So I renamed the foreach version because it has the lower number of uses. Same with the other one you noted below.

if (param.Direction == ParameterDirection.ReturnValue)
{
returnValueParameter = parameter;
returnValueParameter = param;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not keep the full name?

@Wraith2

Wraith2 commented Apr 9, 2023

Copy link
Copy Markdown
Contributor Author

This is green and approved. Can I get it merged before other change start causing conflicts please?

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.

4 participants