Optimize Random{NumberGenerator}.GetItems for power-of-two choices

[stephentoub]

When possible, make a single call to get the required randomness rather than one per element.

Closes #92130

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Security.Cryptography;

BenchmarkSwitcher.FromAssembly(typeof(Tests).Assembly).Run(args);

[HideColumns("Error", "StdDev", "Median", "RatioSD")]
public class Tests
{
    private const int Length = 11;
    private static readonly string s_charChoices = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_-";
    private static ReadOnlySpan<int> s_intChoices => [42, 43, 44, 45, 46, 47, 48, 49];
    private static int[] s_dest = new int[Length];

    [Benchmark]
    public string RngGetString() => RandomNumberGenerator.GetString(s_charChoices, Length);

    [Benchmark]
    public void RandGetItems() => Random.Shared.GetItems(s_intChoices, s_dest);
}

Method	Toolchain	Mean	Ratio
RngGetString	\main\corerun.exe	764.18 ns	1.00
RngGetString	\pr\corerun.exe	98.25 ns	0.13
RandGetItems	\main\corerun.exe	57.21 ns	1.00
RandGetItems	\pr\corerun.exe	16.56 ns	0.29

[ghost]

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

Issue Details

---

When possible, make a single call to get the required randomness rather than one per element.

Closes #92130

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Security.Cryptography;

BenchmarkSwitcher.FromAssembly(typeof(Tests).Assembly).Run(args);

[HideColumns("Error", "StdDev", "Median", "RatioSD")]
public class Tests
{
    private const int Length = 11;
    private static readonly string s_charChoices = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_-";
    private static ReadOnlySpan<int> s_intChoices => [42, 43, 44, 45, 46, 47, 48, 49];
    private static int[] s_dest = new int[Length];

    [Benchmark]
    public string RngGetString() => RandomNumberGenerator.GetString(s_charChoices, Length);

    [Benchmark]
    public void RandGetItems() => Random.Shared.GetItems(s_intChoices, s_dest);
}

Method	Toolchain	Mean	Ratio
RngGetString	\main\corerun.exe	764.18 ns	1.00
RngGetString	\pr\corerun.exe	98.25 ns	0.13
RandGetItems	\main\corerun.exe	57.21 ns	1.00
RandGetItems	\pr\corerun.exe	16.56 ns	0.29

Author:	stephentoub
Assignees:	-
Labels:	area-System.Security
Milestone:	-

[Ponant]

A few remarks if you do not mind.

1. I tested @bartonjs method against mine and confirm it is faster, but it is mainly due to the use bitwise AND instead of integer division, and not the unsafe territory. At least on my machine.

2. I think the benefit of the pow2 non-negligible speedup should be documented in the methods that use it such as GetString, perhaps...

3. I have an addional suggestion, which I think can be beneficial to users and which can be implemented in the same PR. But I do not know if oyu prefer that I create another issue or drop it here? it is about rewritting GetBytes to allow users to have a byte array where each byte is bound to [0,N] where N is a power of two. For N=256, we recover the case of the current GetBytes. Let me know I will drop a code

[stephentoub]

I have an addional suggestion, which I think can be beneficial to users and which can be implemented in the same PR. But I do not know if oyu prefer that I create another issue or drop it here? it is about rewritting GetBytes to allow users to have a byte arrays where each byte is bound to [0,N] where N is a power of two. For N=256, we recover the case of the current getBytes. Let me know I will drop a code

This sounds very niche, and the savings is effectively an array access per element. I don't think it's worth special-casing without seeing more examples of where it would be really meaningful.

I think the benefit of the pow2 non-negligible speedup should be documented in the methods that use it such as GetString, perhaps...

It's an implementation detail. We avoid documenting such optimization implementations, as we reserve the right to change them at any point.

[Ponant]

I have an addional suggestion, which I think can be beneficial to users and which can be implemented in the same PR. But I do not know if oyu prefer that I create another issue or drop it here? it is about rewritting GetBytes to allow users to have a byte arrays where each byte is bound to [0,N] where N is a power of two. For N=256, we recover the case of the current getBytes. Let me know I will drop a code

This sounds very niche, and the savings is effectively an array access per element. I don't think it's worth special-casing without seeing more examples of where it would be really meaningful.

A) Generate random booleans
B) Generate Random vectors in 4 dimensions (my case)

[stephentoub]

I have an addional suggestion, which I think can be beneficial to users and which can be implemented in the same PR. But I do not know if oyu prefer that I create another issue or drop it here? it is about rewritting GetBytes to allow users to have a byte arrays where each byte is bound to [0,N] where N is a power of two. For N=256, we recover the case of the current getBytes. Let me know I will drop a code

This sounds very niche, and the savings is effectively an array access per element. I don't think it's worth special-casing without seeing more examples of where it would be really meaningful.

A) Generate random booleans B) Generate Random vectors in 4 dimensions (my case)

I don't follow. Are you proposing a new API? Or are you proposing that the implementation special-case typeof(T) == typeof(bool) and validates that choices[0] == false and choices[1] == Unsafe.As<byte, bool>(1)? And for typeof(T) == typeof(byte), looping through all of the choices as a precompute step to make sure that choices[i] == (byte)i?

[GrabYourPitchforks]

@stephentoub Consider also duplicating the test here with one that passes a power-of-two sized values span. That way we get distribution tests across both implementations which now exist in this method.

runtime/src/libraries/System.Security.Cryptography/tests/RandomNumberGeneratorTests.cs

Lines 669 to 675 in 0ad4e69

	public static void GetString_RandomDistribution()
	{
	const string Choices = "abcdefghijklmnopqrstuvwxyz";
	string generated = RandomNumberGenerator.GetString(Choices, 10_000);
	VerifyDistribution<char>(generated, 1f / 26);
	VerifyAllInRange(generated, 'a', (char)('z' + 1));
	}

[Ponant]

The current GetBytes returns an array of bytes, where each byte can take any value from 0 to 255. My proposal is to have GetBytes to also allow me to return an array of bytes but where each byte is bound to [0,N] where N is a power of two. For example, I can generate a byte array of bools 011100011010101 or random quaternions or any 4d vector, or octonions or above powers.

I am using something like that, it is essentially the same spirit as you do here with the new GetString in this PR, but for getting byte arrays. I am using something like that (checks on parameters omitted for brievety)

private static byte[] GetRandomBytes(int length, int maxNumOfBytes)
 {
        Span<byte> bytes = length <= _stackAllocThreshold ? stackalloc byte[length] : new byte[length];

        // Generate random bytes, each taking values between 0 and 255
        RandomNumberGenerator.Fill(bytes);

        // We do not need to loop if we cover all byte possibilities, since  RandomNumberGenerator.Fill does this 
        if (maxNumOfBytes != 256)
        {
            byte mask = (byte)(maxNumOfBytes - 1);

            // make each byte between 0 and maxNumOfBytes
            for (var i = 0; i < bytes.Length; i++)
                bytes[i] &= mask;

        }

        return bytes.ToArray();
}

[stephentoub]

The current GetBytes returns an array of bytes, where each byte can take any value from 0 to 255. My proposal is to have GetBytes to also allow me to return an array of bytes but where each byte is bound to [0,N] where N is a power of two.

That's a new API. Feel free to open a separate proposal for one. That won't be done as part of an optimization PR.

[vcsjones]

The current GetBytes returns an array of bytes, where each byte can take any value from 0 to 255. My proposal is to have GetBytes to also allow me to return an array of bytes but where each byte is bound to [0,N] where N is a power of two. For example, I can generate a byte array of bools 011100011010101 or random quaternions or any 4d vector, or octonions or above powers.

This would be a new API proposal.

We can discuss the merits and usefulness of the proposal there to keep this PR focused on the perf of existing APIs.

[stephentoub]

@bartonjs, per your feedback on my recent PR, I've marked this as a breaking change to be documented.

[stephentoub]

Opened #108018 to revert the breaking change aspect.