Skip to content

Simplify collision ratio computation loop in FrozenHashTable.CalcNumBuckets - #131062

Open
prozolic wants to merge 1 commit into
dotnet:mainfrom
prozolic:frozenhashtable
Open

Simplify collision ratio computation loop in FrozenHashTable.CalcNumBuckets#131062
prozolic wants to merge 1 commit into
dotnet:mainfrom
prozolic:frozenhashtable

Conversation

@prozolic

Copy link
Copy Markdown
Contributor

This PR reduces the Array.Clear range on seenBuckets to the minimum number of ints needed for its bitmap (numBuckets / BitsPerInt32 + 1), instead of clearing up to numBuckets ints (Math.Min(numBuckets, seenBuckets.Length)).

Also, it simplifies the loop condition by setting codes to null once all hash codes turn out to be unique (uniqueCodesCount == hashCodes.Length).

…uckets

This PR reduces the `Array.Clear` range on `seenBuckets` to the
minimum number of ints needed for its bitmap
(`numBuckets / BitsPerInt32 + 1`), instead of clearing up to
`numBuckets` ints (`Math.Min(numBuckets, seenBuckets.Length)`).

Also, it simplifies the loop condition by setting `codes` to null
once all hash codes turn out to be unique
(`uniqueCodesCount == hashCodes.Length`).
Copilot AI review requested due to automatic review settings July 20, 2026 06:39
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jul 20, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts FrozenHashTable.CalcNumBuckets to reduce per-iteration work in the prime-try loop by clearing only the portion of the seenBuckets bitmap needed for the current numBuckets, and it simplifies the uniqueness-path branching by setting codes to null when all input hash codes are unique.

Changes:

  • Clear seenBuckets based on the current bucket-count bitmap size rather than Math.Min(numBuckets, seenBuckets.Length).
  • Set codes = null when HashSet construction reveals the input hash codes are unique, simplifying the later loop condition.

@prozolic
prozolic marked this pull request as ready for review July 20, 2026 07:14
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "04d9a6100719706cace50d8032c6af0cc9d88674",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "71d81c1705aa1f00e0b57073c97efdd5496f6b9f",
  "last_reviewed_commit": "04d9a6100719706cace50d8032c6af0cc9d88674",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "71d81c1705aa1f00e0b57073c97efdd5496f6b9f",
  "last_recorded_worker_run_id": "29725959557",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "04d9a6100719706cace50d8032c6af0cc9d88674",
      "review_id": 4733108134
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holistic Review

Motivation: FrozenHashTable.CalcNumBuckets probes candidate prime bucket counts and, for each, clears a bitmap in seenBuckets before counting collisions. The prior code cleared Math.Min(numBuckets, seenBuckets.Length) ints, which over-clears the bitmap by up to 32x (the bitmap only needs numBuckets/32 + 1 ints), and it carried a redundant uniqueCodesCount != hashCodes.Length term in the per-iteration branch that could instead be resolved once up front.

Approach: Two changes. (1) After deduplicating hash codes, when uniqueCodesCount == hashCodes.Length the HashSet offers no benefit, so codes is set to null, collapsing the hot-loop condition to a simple codes is not null check and steering execution to the faster span-iteration path. (2) The Array.Clear length is reduced from Math.Min(numBuckets, seenBuckets.Length) to (numBuckets / BitsPerInt32) + 1, matching the exact number of bitmap words in use.

Summary: Both changes are correct and behavior-preserving. Bucket indices span 0..numBuckets-1, mapping to bitmap words 0..(numBuckets-1)/32; since (numBuckets/32)+1 >= ((numBuckets-1)/32)+1, every word that IsBucketFirstVisit can touch is cleared, and because numBuckets <= maxNumBuckets the cleared length never exceeds the rented (maxNumBuckets/32)+1 capacity, so there is no out-of-bounds or stale-bit risk. Setting codes = null is equivalent to the old compound condition and simplifies the branch without altering results. This is a clean, low-risk optimization with no functional change. LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 43.3 AIC · ⌖ 15.8 AIC · ⊞ 10K

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Collections community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants