Skip to content

Reduce allocation in random set index picking #1242

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

PaulusParssinen
Copy link
Contributor

@PaulusParssinen PaulusParssinen commented Jun 6, 2025

Casting returning type to Span<T> doesn't remove the backing heap allocation. We have to hand the responsibility of allocating the buffer to the caller. Extracted from #1100

We can sort get rid of these constants and calculate the threshold for the constant sized stackalloc on the fly with SpanOwner<T> like utility if this pattern gets more common in Garnet. I'm also using a bit conservative 1K threshold, could be higher if we wish so.

@Copilot Copilot AI review requested due to automatic review settings June 6, 2025 16:19
Copy link
Contributor

@Copilot Copilot AI left a comment

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 refactors the random index picking logic in several object implementations by replacing calls that returned buffer allocations with an approach in which the caller supplies a buffer, thereby reducing heap allocations.

  • Updated SortedSetObjectImpl.cs to conditionally use stackalloc and pass a buffer to RandomUtils.
  • Modified SetObjectImpl.cs and HashObjectImpl.cs similarly to eliminate unneeded allocations.
  • Updated RandomUtils.cs to accept a Span for filling random indexes instead of returning a new array.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
libs/server/Objects/SortedSet/SortedSetObjectImpl.cs Replaced return logic with in-place buffer allocation using stackalloc.
libs/server/Objects/Set/SetObjectImpl.cs Updated random index selection in both count positive and negative cases.
libs/server/Objects/Hash/HashObjectImpl.cs Modified array length usage and in-place buffer population.
libs/common/RandomUtils.cs Changed API to fill a provided Span and updated related helper logic.
Comments suppressed due to low confidence (2)

libs/server/Objects/Set/SetObjectImpl.cs:174

  • The outer declaration of 'indexes' is shadowed by the inner declaration in each branch, which may cause confusion. Consider removing the outer declaration or reusing it to improve clarity.
var indexes = countParameter <= StackallocThreshold ? stackalloc int[StackallocThreshold].Slice(0, countParameter) : new int[countParameter];

libs/common/RandomUtils.cs:76

  • The initialization loop that assigns 'shuffledIndexes[i] = i' is crucial for correct shuffling. Ensure that 'random.Shuffle(shuffledIndexes)' is well-tested to correctly randomize the array after this initialization.
for (var i = 0; i < shuffledIndexes.Length; i++)

@TalZaccai TalZaccai requested review from TedHartMS and TalZaccai June 10, 2025 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants