Optimize performance of SecurityIdentifier.ToString()

[Wraith2]

in SqlClient when using trusted authentication the windows user identity is used as part of the pool key. Profiling has shown that getting the user sid is on the hot path for this and that optimizing the retrieval of the SecurityIdentifier string form will improve connection throughput.

This change avoids the use of a StringBuilder, it's expansion and formatting intermediate strings by using a stackalloc span of the maximum possible length for a v1 string form sid and formatting directly into that buffer. The size calculation is included in the comments so the next person to look at this code doesn't have to work it out for themselves.

The stackalloc buffer is 189 chars in length which looks odd but remember that chars are 16bit so this buffer is still 16bit aligned on the stack. It could be increased up to the closest power of 2 (192) if this is desirable, I know there are rules about this which @GrabYourPitchforks will need to review.

Performance changes make it almost 50% faster and change the memory allocation from using StringBuilder expansion and intermediate formatting representation strings to a flat allocation at the end of the method to return the string.

Method	Mean	Error	StdDev	Ratio	Gen 0/1k Op	Allocated Memory/Op
Reference	829.0 ns	9.954 ns	8.824 ns	1.00	0.4339	1368 B
Development	431.9 ns	3.390 ns	3.171 ns	0.52	0.1140	360 B

/CC @GrabYourPitchforks , @bartonjs
Contributes to https://github.com/dotnet/corefx/issues/30430 @saurabh500

[Wraith2]

The packaging build failure indicates that Span<> isn't found for the netcoreapp2.0 build I think. Any suggestions?

[jkotas]

The packaging build failure indicates that Span<> isn't found for the netcoreapp2.0 build I think. Any suggestions?

Keep the existing code under ifdef for netstandard and netcoreapp2.0 configurations.

Alternatively, we may stop building netstandard and netcoreapp2.0 version of the package. @safern Is it an option for this package?

[stephentoub]

Other than my request about tests, this LGTM. Thanks for doing this.

[safern]

Alternatively, we may stop building netstandard and netcoreapp2.0 version of the package. @safern Is it an option for this package?

It would be an option, yes. However it has its pros and cons:

Pros:

• It would make easier for us to make more perf improvements in the package.
• Build would be definitely simpler for contributors and implementation wouldn't have to be conditionally compiled

Cons:

• It would make bug fixes and servicing more complicated because we would need to backport everything to release branches.
• We would need to freeze API because we would now be harvesting assets.
• Servicing would definitely be slower and more painful.

We should also consider the following options for this package:

• It has an inbox implementation but an OOB reference assembly, so the current way we have it, the package will not contain a netcoreapp3.0/2.1 implementation (it is stated as inbox) but it carries a compatible implementation for them (netcoreapp2.0) which would override the inbox implementation, so we could be having the perf fixes, but don't realize people that reference this package might not even be getting the inbox implementation, but instead the one carried in the package.

• We could make this package inbox in its implementation AND REF, however this package is a TURD assembly for non-Windows implementations. (Also the netstandard implementation is TURD)

So everything comes to the following question:

Do we consider this package API complete to freeze its API? I think for the 3.0 effort we're having some APIs being added to this package (maybe still on PR) so we should make sure this package is API complete before making that call.

From looking at this changes, I would also suggest if we don't want to freeze the API and/or make its ref inbox, currently the way it is. The PR would need the following changes:

• You don't need to if def the changes for netstandard, since netstandard implementation is just a TURD assembly and it is not even using these sources.
• You want to add a netcoreapp or even netcoreapp2.1 package configuration to carry over an implementation within the package containing this perf improvements since the netcoreapp2.0 implemenation would win over the one shipped inbox.

cc: @ericstj

[safern]

Please add a netcoreapp2.1 configuration into configurations.props in the src project. Per: #33579 (comment)
This should be under the PackageConfigurations. So it would look like:

  <PackageConfigurations>
      netstandard;
      netcoreapp2.0-Windows_NT;
      netcoreapp2.0-Unix;
      netcoreapp2.1-Windows_NT;
      netcoreapp2.1-Unix;
      net461-Windows_NT;
    </PackageConfigurations>
    <BuildConfigurations>
      $(PackageConfigurations)
      netcoreapp-Windows_NT;
      netcoreapp-Unix;
      uap-Windows_NT;
      netfx-Windows_NT;
    </BuildConfigurations>

You will also need to update the property in the source project. You can also achieve this by running dotnet msbuild /t:UpdateVSConfigurations in the root of the repo. (This might update other projects though)

Also, no need to ifdef for netstandard: #if netcoreapp20 should be enough, we're not including this sources into the netstandard build.

[Wraith2]

Done.

[safern]

Could you also update the property to include netcoreapp2.1? https://github.com/dotnet/corefx/blob/master/src/System.Security.Principal.Windows/src/System.Security.Principal.Windows.csproj#L9

[Wraith2]

Do you mean add 2.1 variants of all the 2.0 entries in that list?

[safern]

You can just run dotnet msbuild /t:UpdateVSConfigurations in the root of your repo. Then just add the changes to the src\System.Security.Principal.Windows.csproj file to your commit.

[Wraith2]

ok, all done.

[Wraith2]

D:\j\workspace\windows-TGrou---0d2c9ac4\src\Common\src\Interop\Windows\advapi32\Interop.LsaNtStatusToWinError.cs(12,9): error BCL0015: advapi32.dll!LsaNtStatusToWinError is not supported on one\more targeted platforms. [D:\j\workspace\windows-TGrou---0d2c9ac4\src\System.Security.Principal.Windows\src\System.Security.Principal.Windows.csproj]

Looks like it's coming from netcoreapp2.1-Windows_NT. That pinvoke is still in the PinvokeAnalyzerExceptionList.analyzerdata.netcoreapp file the same as it was before I started and which works for the 2.0 version of that target. Any guidance?

[Wraith2]

Any ideas on the interop error?

[safern]

You just need to duplicate this file: https://github.com/dotnet/corefx/blob/master/src/System.Security.Principal.Windows/src/PinvokeAnalyzerExceptionList.analyzerdata.netcoreapp and rename it to PinvokeAnalyzerExceptionList.analyzerdata.netcoreapp2.1

[Wraith2]

Osx build failure is unrelated, i'll request a retest but otherwise it looks done.
@dotnet-bot test OSX x64 Debug Build