Make HashHelpers.Primes private; use GetPrime in FrozenHashTable#126049
Make HashHelpers.Primes private; use GetPrime in FrozenHashTable#126049danmoseley wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-collections |
There was a problem hiding this comment.
Pull request overview
This PR tightens encapsulation around HashHelpers by making the precomputed prime table (Primes) private and updating FrozenHashTable to rely on HashHelpers.GetPrime rather than iterating the table directly.
Changes:
- Make
HashHelpers.Primesprivate inSystem.Private.CoreLibandSystem.Reflection.MetadataLoadContext. - Update
FrozenHashTable.CalcNumBucketsto iterate candidate bucket sizes via repeatedGetPrimecalls instead of scanningPrimesby index. - Update a stale comment in
ObjectIDGeneratorto no longer referenceHashHelpers.Primes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Runtime.Serialization.Formatters/src/System/Runtime/Serialization/ObjectIDGenerator.cs | Updates initialization comment to remove reliance on HashHelpers.Primes. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/HashHelpers.cs | Makes the prime table span private within HashHelpers. |
| src/libraries/System.Private.CoreLib/src/System/Collections/HashHelpers.cs | Makes the prime table span private within CoreLib HashHelpers. |
| src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs | Reworks bucket-count candidate iteration to use GetPrime instead of direct prime-table access. |
src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs
Show resolved
Hide resolved
danmoseley
force-pushed
the
hashhelpers-primes-private
branch
from
83f7496 to
e2b1e1d
Compare
| // If the smallest prime >= minNumBuckets already exceeds our budget, | ||
| // there's no range of primes to search — just use it directly. | ||
| int startPrime = HashHelpers.GetPrime((int)minNumBuckets); | ||
| if (startPrime > maxNumBuckets) | ||
| { | ||
| Debug.Assert(maxPrimeIndexExclusive != 0); | ||
| maxNumBuckets = primes[maxPrimeIndexExclusive - 1]; | ||
| return startPrime; | ||
| } | ||
|
|
There was a problem hiding this comment.
The “very large inputs” comment doesn’t match what the code actually checks. startPrime > maxNumBuckets is effectively unreachable with the current min/max policies (min is ~2x unique, max is >=3x or 16x unique), so this won’t skip the collision-tuning loop when minNumBuckets exceeds the precomputed prime table. Consider adding an explicit guard for the “beyond precomputed primes” case (e.g., via a HashHelpers helper that can detect/limit to the table) and update the comment accordingly.
| for (numBuckets = startPrime; | ||
| numBuckets <= maxNumBuckets; | ||
| numBuckets = HashHelpers.GetPrime(numBuckets + 1)) | ||
| { |
There was a problem hiding this comment.
The for loop advances via HashHelpers.GetPrime(numBuckets + 1). Once numBuckets reaches the end of HashHelpers’ precomputed prime table, GetPrime switches to trial division and this loop can end up iterating over a large number of consecutive primes up to maxNumBuckets (and running the full collision-counting pass each time), which can be prohibitively expensive for large frozen collections. To avoid a potential perf cliff, cap the search to the precomputed table (or a fixed max iteration count) and fall back to a single GetPrime(...) choice when the range goes beyond the table.
HashHelpers.Primes is an implementation detail that was exposed as internal/public. The only external consumer was FrozenHashTable, which iterated the table directly for collision-tuning bucket selection. Replace the direct table access with GetPrime calls in a simple loop. Expose only MaxPrecomputedPrime so callers can bail before hitting trial-division territory. - Make Primes private in System.Private.CoreLib and MetadataLoadContext - Add MaxPrecomputedPrime property derived from the table - Replace index-based primes iteration in FrozenHashTable with GetPrime loop - Add XML doc comments to HashHelpers public methods - Update stale comment in ObjectIDGenerator Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danmoseley
force-pushed
the
hashhelpers-primes-private
branch
from
e2b1e1d to
086472e
Compare
|
abandoning for now. If it matters, I'll pick up sometime after the other PR. |
HashHelpers.Primesis an implementation detail — the precomputed prime table backingGetPrime. It was exposed asinternal(inSystem.Private.CoreLib) andpublic(inMetadataLoadContext), but its only external consumer wasFrozenHashTable, which iterated the table directly for collision-tuning bucket selection.This PR makes
Primesprivate and replaces the direct table access inFrozenHashTablewith a simpleGetPrimeloop.GetPrimealready encapsulates the same table scan, and trial division past the table is negligible on this cold construction path (~1300 divisions at 7M).Changes
Primesprivate inSystem.Private.CoreLibHashHelpersPrimesprivate inMetadataLoadContextHashHelpersFrozenHashTable.CalcNumBucketswithGetPrimeloop (−29 lines)ObjectIDGeneratorNote
This PR was generated with the assistance of GitHub Copilot.