17950 Hash Statics #39216
17950 Hash Statics #39216
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
...stem.Security.Cryptography.Algorithms/src/Internal/Cryptography/HashProviderDispenser.OSX.cs
Show resolved
Hide resolved
src/libraries/Native/Unix/System.Security.Cryptography.Native.Apple/pal_digest.c
Outdated
Show resolved
Hide resolved
| HashAlgorithmNames.SHA384 => Interop.Crypto.EvpSha384(), | ||
| HashAlgorithmNames.SHA512 => Interop.Crypto.EvpSha512(), | ||
| HashAlgorithmNames.MD5 => Interop.Crypto.EvpMd5(), | ||
| _ => throw new CryptographicException(SR.Format(SR.Cryptography_UnknownHashAlgorithm, hashAlgorithmId)) |
There was a problem hiding this comment.
Since these are shared handles we can probably gain some mild wins by memoizing the values in managed (and avoiding the P/Invoke every time to get a redundant answer).
src/libraries/System.Security.Cryptography.Algorithms/tests/HashAlgorithmTest.cs
Outdated
Show resolved
Hide resolved
For just getting it working the current implementation is OK (especially if it lets the PR get merged before tomorrow morning), but it would definitely be good for the static method to just be a right answer, instead of "maybe not the best answer on Windows if allocations are currently a problem for you". Using ThreadStatic or pooling or whatever (with the optional enhancement of using the Win10 one-shot calls) can be done as a followup. |
Is there something interesting happening tomorrow morning? |
|
Marking as ready for review, I should be able to address the Apple implementation and test coverage later this evening. For the rest, if there is a deadline we are trying to hit by tomorrow, I think I follow up PR makes sense. I'll mark this as non-closing the issue. |
If it's in tomorrow morning it's part of preview 8 😄. |
|
The failing legs seem to just be queue timeouts and the like. |
ghost
locked as resolved and limited conversation to collaborators
A draft for hashing statics.
Looking for feedback on approach, particularly CNG's since all I did was end up re-using the existing hash mechanisms. Is it worth pooling instances of those with ThreadStatic or similar?
Work is done in native shims for macOS / Linux to cut down on p/invokes since in this particular case all we'd end up doing is passing around handles to native stuff anyway.
Things that still need to be addressed
Contributes to #17590