Skip to content

Reduce allocations in Md5Hasher and fix broken ThreadLocal<MD5> reuse - #20275

Open
xperiandri wants to merge 1 commit into
dotnet:mainfrom
xperiandri:hashing-improvements
Open

Reduce allocations in Md5Hasher and fix broken ThreadLocal<MD5> reuse#20275
xperiandri wants to merge 1 commit into
dotnet:mainfrom
xperiandri:hashing-improvements

Conversation

@xperiandri

Copy link
Copy Markdown
Contributor

Fixes #20204

Changes

  • Md5Hasher.computeHash now actually reuses the thread-local MD5 instance under NETSTANDARD2_0 (previously a new MD5 instance was created on every call, with a TODO comment noting the ThreadLocal "wasn't working"), and uses MD5.HashData on modern TFMs.
  • Added Md5Hasher.hashStringInto, which hashes a string directly into a caller-supplied 16-byte Span<byte>, renting the UTF8 encoding buffer from ArrayPool<byte>.Shared instead of allocating a new byte array per call.
  • Added Md5Hasher.hashStringToString, producing the hex-string hash of a string without a separate hashString |> toString allocation.
  • hashString is now implemented in terms of hashStringInto.
  • Updated call sites (prim-lexing.fs, FSharpProjectSnapshot.fs) to use the new allocation-reduced helpers.
  • Added release notes entry.

@github-actions

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev


✅ Found changes and release notes in following paths:

Change path Release notes path Description
`src/Compiler` docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@xperiandri xperiandri changed the title Reduce allocations in Md5Hasher and fix broken ThreadLocal&lt;MD5&gt; reuse Reduce allocations in Md5Hasher and fix broken ThreadLocal<MD5> reuse Aug 17, 2026
@xperiandri xperiandri changed the title Reduce allocations in Md5Hasher and fix broken ThreadLocal<MD5> reuse Reduce allocations in Md5Hasher and fix broken ThreadLocal<MD5> reuse Aug 17, 2026
@xperiandri
xperiandri force-pushed the hashing-improvements branch from d3f950c to 2219dc8 Compare August 17, 2026 01:26
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Aug 17, 2026

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice cleanup — the ThreadLocal<MD5> is finally doing its job, and the pooled encode buffer plus the no-copy AsImmutableArray wrap remove real per-checksum allocations on a hot path.

Verified the parts that could have bitten:

  • hashStringToString keeps BitConverter.ToString's dashed format, so FSharpProjectSnapshot version strings are unchanged.
  • The AsImmutableArray wrap is safe here: both GetChecksum sites hand it a freshly-allocated 16-byte local that's never aliased afterwards.
  • ImmutableCollectionsMarshal.AsImmutableArray is unguarded but ships in the netstandard2.0 target of System.Collections.Immutable since 8.0.0, and the repo floor is 10.0.9, so both TFMs compile.
  • Md5Hasher.hashString is fully removed with no remaining callers, and Md5Hasher is internal, so no surface-area impact.

Two optional nits, neither blocking:

  • The #else path ignores MD5.TryHashData's bool result. It can't fail with an exact 16-byte destination, but an assert/invalidOp on false would document that invariant.
  • The XML-doc blocks are a touch long for a facilities helper; a one-liner each would read fine.

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Aug 17, 2026
@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Aug 17, 2026
@T-Gro
T-Gro self-requested a review August 17, 2026 08:34
@xperiandri
xperiandri force-pushed the hashing-improvements branch 2 times, most recently from b7c5260 to 4447fb4 Compare August 17, 2026 11:45
@xperiandri
xperiandri force-pushed the hashing-improvements branch from 4447fb4 to 8d9043c Compare August 17, 2026 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

Hashing: avoid intermediate allocations in Md5Hasher and fix broken ThreadLocal usage under NETSTANDARD2_0

2 participants