Skip to content
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

Checksum's usage of a native hash algorithm resulting in thread starvation #67995

Closed
davkean opened this issue Apr 27, 2023 · 6 comments · Fixed by #67999
Closed

Checksum's usage of a native hash algorithm resulting in thread starvation #67995

davkean opened this issue Apr 27, 2023 · 6 comments · Fixed by #67999
Labels
untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@davkean
Copy link
Member

davkean commented Apr 27, 2023

Version Used:
Investigating thread starvation in Speedometer's CopyRichNotAccurate, I see Checksum's usage of CAPI SHA256 hash algorithm is one of the cause of the starvations. It appears the following code path instantiates large number of finalizable objects (ie the underlying SafeHandle) on multiple threads at the same in a short burst. This hits a lock in the CLR's CFinalize::RegisterForFinalization causing contention and resulting in a lot of blocked time on the thread pool, 10% of the blocked time on these threads is this bug.

Steps to Reproduce:

  1. Download trace from any run of Speedometer's CopyRichNotAccurate tests (ask me offline for how to do this)
  2. Look at blocked time via Thread Time Stacks under CFinalize::RegisterForFinalization.

image

I had a look at SHA265Cng and it has a similar path so is likely not the path forward. Maybe the managed version of the algorithm? Or alternative, a custom wrapper around CNG that avoids creating a finalizable safe handle over and over again?

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Apr 27, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@CyrusNajmabadi
Copy link
Member

@stephentoub any suggestions on a good way to utilize this api while avoiding this problem?

@sharwell
Copy link
Member

Based on the time shown above and the fact that this is already correctly performing on a background thread, I'm OK with leaving this unchanged if any of the following is true:

  • The underlying issue has already been fixed on .NET, and only affects .NET Framework scenarios
  • An API change is needed to fix this, but the API isn't available on .NET Framework
  • There is no available API to avoid this lock

@stephentoub
Copy link
Member

stephentoub commented Apr 27, 2023

On .NET 5+ you can use the one-shot methods... zero allocation (finalizable or otherwise):
image
These are the way to go when targeting .NET 5+ surface area.

If you're stuck on .NET Framework, you can at least use SHA256.Create(), which unless FIPS is enabled will create a SHA256Managed instance and, while there will be a lot of allocation:
image
I don't believe any of it will be finalizable (though I'd need to check).

It looks like IncrementalHash is being used by Roslyn. IncrementalHash is just a thin wrapper around the underlying HashAlgorithm instance, and it's choosing the CNG implementations provided by the OS:
image
so you could just do what it does manually with your SHA256 instance: each time you use it for a new operation, call Initialize; call TransformBlock for each block of data (with a null output), and then when you want the result call TransformFinalBlock with an empty array and access the resulting Hash.
image

@sharwell
Copy link
Member

@CyrusNajmabadi Sounds like RoslynIncrementalHash is about to exist. :)

sharwell added a commit to sharwell/roslyn that referenced this issue Apr 27, 2023
@CyrusNajmabadi
Copy link
Member

Yup. I'll work on this today.

sharwell added a commit to sharwell/roslyn that referenced this issue Apr 27, 2023
sharwell added a commit to sharwell/roslyn that referenced this issue Apr 27, 2023
DustinCampbell added a commit to DustinCampbell/razor that referenced this issue Oct 17, 2023
On .NET Framework, IncrementalHash uses the OS's implementation of
SHA-256, which results in several safe handles being created. These
handles are finalizable and can cause lock contention in the CLR if
created rapidly, which Razor does.

This change adjusts the Checksum.Builder to use the SHA256 class
directly on .NET Framework (and netstandard2.0), which causes the
managed implementation of SHA256 to be used (if FIPS isn't enabled).
That implementation avoids the creation of finalizable safe handles.

See dotnet/roslyn#67995 for more detail.
DustinCampbell added a commit to dotnet/razor that referenced this issue Oct 23, 2023
On .NET Framework, IncrementalHash uses the OS's implementation of
SHA-256, which results in several safe handles being created. These
handles are finalizable and can cause lock contention in the CLR if
created rapidly, which Razor does.

This change adjusts the Checksum.Builder to use the SHA256 class
directly on .NET Framework (and netstandard2.0), which causes the
managed implementation of SHA256 to be used (if FIPS isn't enabled).
That implementation avoids the creation of finalizable safe handles.

See dotnet/roslyn#67995 for more detail.

Many thanks to @sharwell for pointing out this issue along with the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
4 participants