Skip to content

Improve perf of hash one-shots on Win10#40510

Merged
bartonjs merged 2 commits intodotnet:masterfrom
GrabYourPitchforks:hash_perf_dev
Aug 13, 2020
Merged

Improve perf of hash one-shots on Win10#40510
bartonjs merged 2 commits intodotnet:masterfrom
GrabYourPitchforks:hash_perf_dev

Conversation

@GrabYourPitchforks
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks commented Aug 7, 2020

Uses bcrypt pseudo-handles (see https://docs.microsoft.com/windows/win32/seccng/cng-algorithm-pseudo-handles) with the BCryptHash routine to get a modest perf improvement in the one-shot hash algortihms when running on Windows 10. For downlevel Windows platforms, continues to use the existing "catch EntryPointNotFound and fall back to BCryptHashData" logic.

The Win10-specific routine also has a safety net such that if any of the invariants are violated (unknown algorithm or destination too short), it falls back to the downlevel code paths for additional error handling.

No change on other OSes.

Method Toolchain HashAlgorithm SourceLengthInBytes Mean Error StdDev Ratio
HashData master SHA1 0 336.4 ns 0.71 ns 0.66 ns 1.00
HashData proto SHA1 0 288.8 ns 2.17 ns 1.81 ns 0.86
HashData master SHA1 64 420.8 ns 2.22 ns 1.85 ns 1.00
HashData proto SHA1 64 374.6 ns 1.35 ns 1.27 ns 0.89
HashData master SHA1 256 651.0 ns 3.40 ns 3.02 ns 1.00
HashData proto SHA1 256 609.0 ns 4.11 ns 3.85 ns 0.94
HashData master SHA256 0 274.7 ns 1.33 ns 1.24 ns 1.00
HashData proto SHA256 0 226.2 ns 1.13 ns 0.94 ns 0.82
HashData master SHA256 64 302.8 ns 1.53 ns 1.43 ns 1.00
HashData proto SHA256 64 255.2 ns 0.51 ns 0.45 ns 0.84
HashData master SHA256 256 390.4 ns 1.10 ns 0.92 ns 1.00
HashData proto SHA256 256 348.5 ns 1.90 ns 1.78 ns 0.89
HashData master SHA512 0 493.0 ns 2.43 ns 2.27 ns 1.00
HashData proto SHA512 0 455.6 ns 1.32 ns 1.10 ns 0.92
HashData master SHA512 64 498.4 ns 1.02 ns 0.91 ns 1.00
HashData proto SHA512 64 468.5 ns 2.48 ns 2.07 ns 0.94
HashData master SHA512 256 947.0 ns 4.13 ns 3.86 ns 1.00
HashData proto SHA512 256 907.9 ns 4.28 ns 4.01 ns 0.96

@ghost
Copy link

ghost commented Aug 7, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
See info in area-owners.md if you want to be subscribed.

@GrabYourPitchforks GrabYourPitchforks added the tenet-performance Performance related issue label Aug 7, 2020
vcsjones
vcsjones reviewed Aug 7, 2020
View reviewed changes
Copy link
Member

@vcsjones vcsjones left a comment

Choose a reason for hiding this comment

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

Neat! Thank you!

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Aug 8, 2020

@vcsjones I addressed your second feedback item by querying the hash size right after we create the algorithm, then shoving that information into the algorithm handle cache. It means we only take the lookup hit once. For the one-shot algos, I think it's still appropriate to hard-code the expected digest length since we need to maintain the friendly-name -> pseudo-alg handle mapping ourselves anyway.

@GrabYourPitchforks GrabYourPitchforks mentioned this pull request Aug 8, 2020
Closed
@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Aug 8, 2020

CI failures are tracked by #40416 and #40564.

vcsjones
vcsjones reviewed Aug 8, 2020
View reviewed changes
....Security.Cryptography.Algorithms/src/Internal/Cryptography/HashProviderDispenser.Windows.cs

if (!s_useCompatOneShot)
NTSTATUS ntStatus;
fixed (byte* pSrc = &MemoryMarshal.GetReference(source))
Copy link
Member

@vcsjones vcsjones Aug 8, 2020

Choose a reason for hiding this comment

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

A question: why the change to use MemoryMarshal.GetReference?

Copy link
Member Author

@GrabYourPitchforks GrabYourPitchforks Aug 8, 2020

Choose a reason for hiding this comment

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

Habit, muscle memory, whatever you want to call it. :)

Copy link
Member

@gfoidl gfoidl Aug 9, 2020

Choose a reason for hiding this comment

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

The way via GetPinnableReference has an extra length check, that can be avoided here, as we know the span won't be empty. So the codegen is better (maybe the reason for Levi's muscle memory to kick in 😉).

@GrabYourPitchforks GrabYourPitchforks mentioned this pull request Aug 8, 2020
Merged
bartonjs
bartonjs approved these changes Aug 12, 2020
View reviewed changes
Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

Seems like a lot of work and complexity for a relatively trivial gain, but since it's written... OK.

@bartonjs
Copy link
Member

bartonjs commented Aug 13, 2020 

    System.Net.Http.Functional.Tests.SocketsHttpHandler_ConnectionFactoryTest.CustomConnectionFactory_SyncRequest_Fails [FAIL]
      Assert.Equal() Failure
      Expected: HostNotFound
      Actual:   Unknown
      Stack Trace:
        /_/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs(165,0): at System.Net.Http.Functional.Tests.SocketsHttpHandler_ConnectionFactoryTest.CustomConnectionFactory_SyncRequest_Fails()
        /_/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs(165,0): at System.Net.Http.Functional.Tests.SocketsHttpHandler_ConnectionFactoryTest.CustomConnectionFactory_SyncRequest_Fails()
        --- End of stack trace from previous location ---
    System.Net.Http.Functional.Tests.SyncHttpHandler_HttpClientHandler_Authentication_Test.Proxy_DomainJoinedProxyServerUsesKerberos_Success [SKIP]
      Condition(s) not met: "IsDomainJoinedServerAvailable"
    System.Net.Http.Functional.Tests.SocketsHttpHandler_ConnectionFactoryTest_Http2.CustomConnectionFactory_SyncRequest_Fails [FAIL]
      Assert.Equal() Failure
      Expected: HostNotFound
      Actual:   Unknown
      Stack Trace:
        /_/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs(165,0): at System.Net.Http.Functional.Tests.SocketsHttpHandler_ConnectionFactoryTest.CustomConnectionFactory_SyncRequest_Fails()
        /_/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs(165,0): at System.Net.Http.Functional.Tests.SocketsHttpHandler_ConnectionFactoryTest.CustomConnectionFactory_SyncRequest_Fails()
        --- End of stack trace from previous location ---

This same test (or a very similar looking one) has failed on at least 3 reruns for the last commit of the PR. Looks like it only happens on Win7, and I haven't seen it on other PRs (but maybe I'm unobservant).

I don't see how it could necessarily be related, but the consistency bothers me.

@vcsjones
Copy link
Member

vcsjones commented Aug 13, 2020

@bartonjs maybe the PR just needs to merge in master? #40564

@bartonjs
Copy link
Member

bartonjs commented Aug 13, 2020

Odd, I thought that every test rerun tried stacking the PR diff on top of master freshly... but I agree with your assessment that this is a known/fixed issue.

🤷

@bartonjs bartonjs merged commit edebfbd into dotnet:master Aug 13, 2020
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@vcsjones vcsjones vcsjones approved these changes

@bartonjs bartonjs bartonjs approved these changes

@krwq krwq Awaiting requested review from krwq

+1 more reviewer

@gfoidl gfoidl gfoidl left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Security tenet-performance Performance related issue

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@GrabYourPitchforks @bartonjs @vcsjones @gfoidl @karelz