Improve speed of Randomizer.GetString

[CollinAlpert]

This PR improves speed and reduces allocations of the Randomizer.GetString method. Here is a benchmark:

Benchmark code:

[MemoryDiagnoser]
public class Benchmark
{
	private const string DefaultStringChars = "abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNOPQRSTUVWXYZ0123456789_";

	[Params(5, 10, 100, 1000)]
	public int OutputLength;

	[Benchmark(Baseline = true)]
	public string GetRandomStringOld()
	{
		var data = new char[OutputLength];

		for (int i = 0; i < data.Length; i++)
			data[i] = DefaultStringChars[Random.Shared.Next(0, DefaultStringChars.Length)];

		return new string(data);
	}

	[Benchmark]
	public string GetRandomStringNew()
	{
		return string.Create(OutputLength, DefaultStringChars, static (span, allowedChars) =>
		{
			for (var i = 0; i < span.Length; i++)
			{
				span[i] = allowedChars[Random.Shared.Next(0, allowedChars.Length)];
			}
		});
	}
}

[manfred-brands]

Please use int iso var for i
Also of you use the same names for the parameters as the other version makes this look similar.

I wonder if you can make a local function for the loop as that is exactly the same between both versions.

[CollinAlpert]

I renamed the parameters and used int.

As for making a local function, that would add an extra allocation for the closure and we wouldn't get as much of a performance benefit.

[OsirisTerje]

What if you make it local and mark it with the MethodImpAttribute(MethodImplOptions.AggressiveInlining) ? Does that affect performance?

And, you measure this in release, right?

Sorry for being a bit outdated: The inline keyword should be enough, and then the code can be beautiful and still performant :-)

[CollinAlpert]

That might help with the allocation (I'd need to check if the JIT would actually be able to inline the function), but that still wouldn't solve the problem of finding a common type between Span<char> and char[]. Any magic performed to put both of those types in a common function will definitely result in worse performance.

[manfred-brands]

A char[] can be assigned to a Span<char> also on .NET Framework. So both could use Span<char>.
I don't know if that changes any optimizations done by the JITter.

Could you put the below in your benchmark please, prevents me from having to write one.

public string GetString(int outputLength, string allowedChars)
{
#if NET6_0_OR_GREATER
    return string.Create(outputLength, allowedChars, Fill);
#else
    char[] data = new char[outputLength];
    Fill(data, allowedChars);
    return new string(data);
#endif

    void Fill(Span<char> data, string allowed)
    {
        for (int i = 0; i < data.Length; i++)
            data[i] = allowed[Next(0, allowed.Length)];
    }
}

[CollinAlpert]

However I have just run a benchmark with the updated code and it seems the method is not inlined and the performance is a bit worse than the NetFx code:

Please note that I do not have net472 installed and am running the net472 code on net8.0, so the string.Create on >= net6.0 version will still be faster than the stackalloc version on net472. It will just not be as performant as it could be.

[manfred-brands]

Not sure what you are comparing here? .NET Core vs NET Framework or old vs new?

[CollinAlpert]

I have no way of providing a realistic benchmark, since I can't install net472. In these benchmarks I am comparing the PR code for net472 and >= net6.0, but running both benchmarks on net8.0. Unfortunately that's the best I can do.

[manfred-brands]

My benchmark results show that for .NET48 nor NET6.0 the new code is not improving things or in case of .NET48 even making things slower, but for NET8.0 the improvement is a lot.

What I haven't tested is to see if a .NET6.0 build of NUNIT when run under NET8.0 would give the same improvement as we are not intending to make an NET8.0 binary Nuget package.

[CollinAlpert]

What does your benchmark do for GetStringNew in net48, since it can't use the string.Create API? Does it use stackalloc?

I managed to run some benchmarks on a Windows machine and also noticed that stackalloc was slower than char[] on net48, so I will probably revert that change.

[OsirisTerje]

This seems to have gone stale.

Does this help in any way now, or should it be closed?