Skip to content

Speeding up Random.Shuffle by batching #111015

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

Closed
wants to merge 5 commits into from
Closed

Speeding up Random.Shuffle by batching #111015

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f9a70c1
Speeding up `Random.Shuffle` by batching
andanteyk Jan 1, 2025
1d08e67
Safe swap, add comment
andanteyk Jan 2, 2025
4d41ce6
Add comments
andanteyk Jan 3, 2025
904aa9d
Add statistical tests to `RandomNumberGenerator.Shuffle()`
andanteyk Jan 4, 2025
8ff424e
Moved Shuffle code to Impl
andanteyk Jan 7, 2025
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
76 changes: 69 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 @@ -363,17 +363,79 @@ public void Shuffle<T>(T[] values)
/// </remarks>
public void Shuffle<T>(Span<T> values)
{
int n = values.Length;
if (_impl is XoshiroImpl xoshiro)
Copy link
Member

@huoyaoyuan huoyaoyuan Jan 1, 2025

Choose a reason for hiding this comment

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

The pattern is not so straight to embed implementation details of XoshiroImpl in base class. Let's see if others have better idea.

Copy link
Author

@andanteyk andanteyk Jan 1, 2025

Choose a reason for hiding this comment

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

I'm also seeing problems.
This implementation requires NextUInt64(), so currently XoshiroImpl is required.

Copy link
Member

@stephentoub stephentoub Jan 6, 2025

Choose a reason for hiding this comment

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

I'd rather ImplBase have a virtual or abstract Shuffle method on it, which this public Shuffle would just delegate to on _impl. Then the XoshiroImpl-specific implementation can be an override.

{
ulong bound = 2432902008176640000; // 20!
int nextIndex = Math.Min(20, values.Length);

for (int i = 1; i < values.Length;)
{
ulong r = xoshiro.NextUInt64();

// Correct r to be unbiased.
// Ensure that the result of `Math.BigMul(r, bound, out _)` is
// uniformly distributed between 0 <= r < bound without bias.
// For details, see https://github.com/dotnet/runtime/pull/111015
ulong rbound = r * bound;
if (rbound > 0 - bound)
{
ulong sum, carry;
do
{
ulong r2 = xoshiro.NextUInt64();
ulong lohi = Math.BigMul(r2, bound, out ulong lolo);
sum = rbound + lohi;
carry = sum < rbound ? 1ul : 0ul;
rbound = lolo;
} while (sum == ~0ul);
r += carry;
}

// Shuffle the values ​​based on r
for (int m = i; m < nextIndex; m++)
{
int index = (int)Math.BigMul(r, (ulong)(m + 1), out r);

// Swap span[m] <-> span[index]
var temp = values[m];
values[m] = values[index];
values[index] = temp;
}

i = nextIndex;

// Calculates bound.
// bound is (i + 1) * (i + 2) * ... * (nextIndex) < 2^64
bound = (ulong)(i + 1);
for (nextIndex = i + 1; nextIndex < values.Length; nextIndex++)
{
if (Math.BigMul(bound, (ulong)(nextIndex + 1), out var newbound) == 0)
{
bound = newbound;
}
else
{
break;
}
}
}

for (int i = 0; i < n - 1; i++)
}
else
{
int j = Next(i, n);
// Fallback & compatibility implementation
int n = values.Length;

if (j != i)
for (int i = 0; i < n - 1; i++)
{
T temp = values[i];
values[i] = values[j];
values[j] = temp;
int j = Next(i, n);

if (j != i)
{
T temp = values[i];
values[i] = values[j];
values[j] = temp;
}
}
}
}
Expand Down
59 changes: 52 additions & 7 deletions ...es/System.Security.Cryptography/src/System/Security/Cryptography/RandomNumberGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Numerics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System.Security.Cryptography
Expand Down Expand Up @@ -288,17 +289,61 @@ public static string GetHexString(int stringLength, bool lowercase = false)
/// <typeparam name="T">The type of span.</typeparam>
public static void Shuffle<T>(Span<T> values)
Copy link
Member

@huoyaoyuan huoyaoyuan Jan 1, 2025

Choose a reason for hiding this comment

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

Changes for cryptographic RNG may subject to more careful review for security requirements.

{
int n = values.Length;
ulong bound = 2432902008176640000; // 20!
int nextIndex = Math.Min(20, values.Length);

for (int i = 0; i < n - 1; i++)
for (int i = 1; i < values.Length;)
{
int j = GetInt32(i, n);
ulong r = 0;
RandomNumberGeneratorImplementation.FillSpan(MemoryMarshal.AsBytes(new Span<ulong>(ref r)));

// Correct r to be unbiased.
// Ensure that the result of `Math.BigMul(r, bound, out _)` is
// uniformly distributed between 0 <= r < bound without bias.
// For details, see https://github.com/dotnet/runtime/pull/111015
ulong rbound = r * bound;
if (rbound > 0 - bound)
{
ulong sum, carry;
do
{
ulong r2 = 0;
RandomNumberGeneratorImplementation.FillSpan(MemoryMarshal.AsBytes(new Span<ulong>(ref r2)));

ulong lohi = Math.BigMul(r2, bound, out ulong lolo);
sum = rbound + lohi;
carry = sum < rbound ? 1ul : 0ul;
rbound = lolo;
} while (sum == ~0ul);
r += carry;
}

// Shuffle the values ​​based on r
for (int m = i; m < nextIndex; m++)
{
int index = (int)Math.BigMul(r, (ulong)(m + 1), out r);

// Swap span[m] <-> span[index]
var temp = values[m];
Copy link
Member

@bartonjs bartonjs Jan 3, 2025

Choose a reason for hiding this comment

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

Suggested change
var temp = values[m];
T temp = values[m];

values[m] = values[index];
values[index] = temp;
}

i = nextIndex;

if (i != j)
// Calculates bound.
// bound is (i + 1) * (i + 2) * ... * (nextIndex) < 2^64
bound = (ulong)(i + 1);
for (nextIndex = i + 1; nextIndex < values.Length; nextIndex++)
{
T temp = values[i];
values[i] = values[j];
values[j] = temp;
if (Math.BigMul(bound, (ulong)(nextIndex + 1), out var newbound) == 0)
{
bound = newbound;
}
else
{
break;
}
}
}
}
Expand Down
Loading