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.

Sign up for GitHub

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

Performance improvements for Random.Shuffle() #83305

Closed
wants to merge 7 commits into from
Closed

Performance improvements for Random.Shuffle() #83305

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6307d29
Performance improvements for Random.Shuffle()
wainwrightmark Mar 11, 2023
262e466
Merge branch 'dotnet:main' into shuffle-performance
wainwrightmark Jun 12, 2023
7895efd
Apply suggestions from code review
wainwrightmark Jun 13, 2023
d94eb2e
Apply suggestions from code review
wainwrightmark Jun 13, 2023
5e54371
Introduced assertions for thresholds in IncreasingUniform
wainwrightmark Jun 14, 2023
60c7fce
Use Pearson's chi-squared test for Shuffle_Array_Fairness
wainwrightmark Jun 14, 2023
90893c5
Fixed indentation
wainwrightmark Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 100 additions & 7 deletions src/libraries/System.Private.CoreLib/src/System/Random.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,18 +273,111 @@ public void Shuffle<T>(T[] values)
/// </remarks>
public void Shuffle<T>(Span<T> values)
{
int n = values.Length;
var chooser = new IncreasingUniform(this);
for (int i = 1; i < values.Length; i++)
{
int index = chooser.NextIndex();
(values[i], values[index]) = (values[index], values[i]);
wainwrightmark marked this conversation as resolved.
Show resolved Hide resolved
}
}

private ref struct IncreasingUniform
wainwrightmark marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly Random _random;
private int _n;
private int _chunk;
private int _chunkRemaining;

for (int i = 0; i < n - 1; i++)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
wainwrightmark marked this conversation as resolved.
Show resolved Hide resolved
public IncreasingUniform(Random random)
{
int j = Next(i, n);
_random = random;
_chunk = 0;
_n = 1;
_chunkRemaining = 0;
wainwrightmark marked this conversation as resolved.
Show resolved Hide resolved
}

if (j != i)
/// <summary>
/// Increase n by one and then return a random positive integer less than the new n
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public int NextIndex()
{
int nextN = _n + 1;
if (_chunkRemaining == 0)
{
T temp = values[i];
values[i] = values[j];
values[j] = temp;
(int bound, int remaining) = CalculateBound(nextN);
_chunkRemaining = remaining - 1;
_chunk = _random.Next(bound);
}
else
{
_chunkRemaining -= 1;
wainwrightmark marked this conversation as resolved.
Show resolved Hide resolved
}

int result;
if (_chunkRemaining == 0)
{
result = _chunk;
}
else
{
result = (_chunk % nextN);
_chunk /= nextN;
}


_n = nextN;
return result;
}

/// <summary>
/// Calculate the highest (x,k) such that x = n*n+1*..*n+k-1 where x is a 32 bit integer.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static (int, int) CalculateBound(int n)
{
int count;
switch (n)
Copy link
Member

Choose a reason for hiding this comment

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

can this switch use fancy new pattern matching syntax to be more compact?

Copy link
Author

Choose a reason for hiding this comment

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

I tried this but I don't think relational patterns allow return statements.

{
case 2:
return (479001600, 11); //12 factorial
Copy link
Member

Choose a reason for hiding this comment

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

would it be worth adding some debug asserts to calculate and self document these magic numbers?

Copy link
Author

@wainwrightmark wainwrightmark Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have done this. I may have gone a bit overboard though - the simplest way to check the threshold numbers is to check that using the next number up results in an overflow exception. How critical is performance with debug symbols?

case 13:
return (253955520, 7); //19 factorial / 12 factorial
wainwrightmark marked this conversation as resolved.
Show resolved Hide resolved
case < 34:
count = 6;
break;
case < 72:
count = 5;
break;
case < 214:
count = 4;
break;
case < 1290:
count = 3;
break;
case < 46341:
count = 2;
break;
default:
return (n, 1);
}

int product = GetProduct(n, count - 1);
return (product, count);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int GetProduct(int n, int multiplications)
{
int product = n;
while (multiplications > 0)
{
product *= (n + multiplications);
multiplications--;
}

return product;
}
}

Expand Down
46 changes: 40 additions & 6 deletions src/libraries/System.Runtime.Extensions/tests/System/Random.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,9 +713,9 @@ public static void Shuffle_Array_Seeded(bool emptyShuffle)
Random random = new Random(0x70636A61);
int[] items = new int[] { 1, 2, 3, 4 };
random.Shuffle(items);
AssertExtensions.SequenceEqual(stackalloc int[] { 4, 2, 1, 3 }, items);
AssertExtensions.SequenceEqual(stackalloc int[] { 1, 3, 4, 2 }, items);
random.Shuffle(items);
AssertExtensions.SequenceEqual(stackalloc int[] { 2, 3, 4, 1 }, items);
AssertExtensions.SequenceEqual(stackalloc int[] { 3, 1, 2, 4 }, items);

if (emptyShuffle)
{
Expand All @@ -724,7 +724,7 @@ public static void Shuffle_Array_Seeded(bool emptyShuffle)
}

random.Shuffle(items);
AssertExtensions.SequenceEqual(stackalloc int[] { 1, 4, 3, 2 }, items);
AssertExtensions.SequenceEqual(stackalloc int[] { 2, 1, 4, 3 }, items);
}

[Fact]
Expand All @@ -742,9 +742,9 @@ public static void Shuffle_Span_Seeded(bool emptyShuffle)
Random random = new Random(0x70636A61);
Span<int> items = new int[] { 1, 2, 3, 4 };
random.Shuffle(items);
AssertExtensions.SequenceEqual(stackalloc int[] { 4, 2, 1, 3 }, items);
AssertExtensions.SequenceEqual(stackalloc int[] { 1, 3, 4, 2 }, items);
random.Shuffle(items);
AssertExtensions.SequenceEqual(stackalloc int[] { 2, 3, 4, 1 }, items);
AssertExtensions.SequenceEqual(stackalloc int[] { 3, 1, 2, 4 }, items);

if (emptyShuffle)
{
Expand All @@ -753,7 +753,41 @@ public static void Shuffle_Span_Seeded(bool emptyShuffle)
}

random.Shuffle(items);
AssertExtensions.SequenceEqual(stackalloc int[] { 1, 4, 3, 2 }, items);
AssertExtensions.SequenceEqual(stackalloc int[] { 2, 1, 4, 3 }, items);
}

[Fact]
public static void Shuffle_Array_Fairness()
{
// Test that repeatedly shuffling an array puts each value in each position a roughly equal number of times.
// This does not prove that the shuffle is fair but will at least fail if the shuffle is obviously unfair
const int runs = 10000;
const int len = 50;
wainwrightmark marked this conversation as resolved.
Show resolved Hide resolved
int[,] counts = new int[len, len];
var rng = new Random(123);
for (int i = 0; i < runs; i++)
{
int[]? array = new int[len];
for (int index = 0; index < len; index++)
{
// Fill the array with the numbers 0..len
array[index] = index;
}

rng.Shuffle(array);

for (int j = 0; j < len; j++)
{
int index = array[j];
counts[j,index] += 1;
wainwrightmark marked this conversation as resolved.
Show resolved Hide resolved
}
}

const int expected = runs / len;
foreach (int count in counts)
{
Assert.InRange(count, expected / 10 * 7, expected * 13 / 10);
wainwrightmark marked this conversation as resolved.
Show resolved Hide resolved
}
}

[Fact]
Expand Down