Reduce allocations in string.Normalize

[MihaZupan]

While inherently an allocatey API (string Normalize(string)), we can avoid allocating the temporary char[] buffer for the P/Invoke.

These char[] allocations account for ~2% of allocated bytes when using HttpClient with a non-ascii Uri in my simple test. The allocated result (even if unchanged) another ~1.5%.

Is there a common stackalloc threshold we should agree upon across runtime? Highest I've seen so far is 1024 chars. I used 512 here as that is what IdnMapping uses as well.

[ghost]

Tagging @tarekgh, @safern as an area owner

[danmoseley]

Is there a common stackalloc threshold we should agree upon across runtime? Highest I've seen so far is 1024 chars. I used 512 here as that is what IdnMapping uses as well.

This one is 4096 (bytes, not chars)

runtime/src/libraries/Common/src/Interop/OSX/Interop.libproc.cs

Line 439 in 719d627

byte* pBuffer = stackalloc byte[PROC_PIDPATHINFO_MAXSIZE];

I assume we should pick a number that is suitable everywhere since we generally do not know how much stack we have - and presumably it's the same number on all platforms for simplicity.

@stephentoub are you comfortable with 4096? 1024?

[stephentoub]

We've generally tried to not go above 1K. 4K feels like a lot, especially on macOS where the default stack size is lower than other platforms. In some cases there's a natural upper-bound based on the scenario, and we've been a bit more liberal in those cases. In others where it's arbitrary, we've talked about just standardizing on a size that we use everywhere, e.g. 256 bytes or 128 chars. I would still like us to go through and do such a standardization where appropriate, we've just not prioritized it.
cc: @jkotas

[jkotas]

I would still like us to go through and do such a standardization where appropriate, we've just not prioritized it.

It should be a new API that will do the right thing for the given context and platform without your worrying about it: #25423

[GrabYourPitchforks]

Heads up: we generally don't return buffers to the shared pool inside finally blocks. The sole exception is where we're in 100% control of every single code path that might be invoked inside the preceding try.

[stephentoub]

Heads up: we generally don't return buffers to the shared pool inside finally blocks. The sole exception is where we're in 100% control of every single code path that might be invoked inside the preceding try.

We're inconsistent on this, e.g.

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs

Lines 114 to 130 in 26b6e4e

	private async Task CopyToAsyncInternal(Stream destination, int bufferSize, CancellationToken cancellationToken)
	{
	byte[] buffer = ArrayPool<byte>.Shared.Rent(bufferSize);
	try
	{
	while (true)
	{
	int bytesRead = await ReadAsync(new Memory<byte>(buffer), cancellationToken).ConfigureAwait(false);
	if (bytesRead == 0) break;
	await destination.WriteAsync(new ReadOnlyMemory<byte>(buffer, 0, bytesRead), cancellationToken).ConfigureAwait(false);
	}
	}
	finally
	{
	ArrayPool<byte>.Shared.Return(buffer);
	}
	}

[GrabYourPitchforks]

I know, but we should try to get it under check for new code (like this). The GitHub UI is misbehaving for me so I can't see the rest of the code. So this usage might not be problematic at all but somebody should double check.

[MihaZupan]

Added a check to return the original string if normalization didn't change anything (which should be the common-case as well)

[stephentoub]

Is this valid even if realLength > 0?

[stephentoub]

(I see the above comment now)

[stephentoub]

One question about comparing to Interop.BOOL.TRUE. Otherwise LGTM.