Reduce allocations in string.Normalize by MihaZupan · Pull Request #34774 · dotnet/runtime

MihaZupan · 2020-04-09T18:02:53Z

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 · 2020-04-09T18:02:55Z

Tagging @tarekgh, @safern as an area owner

src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Normalization.cs

danmoseley · 2020-04-09T18:12:54Z

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 · 2020-04-09T18:16:31Z

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

src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Normalization.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Unix.cs

src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Normalization.cs

jkotas · 2020-04-09T18:34:14Z

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

src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Normalization.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Windows.cs

src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Normalization.cs

GrabYourPitchforks · 2020-04-09T19:33:19Z

src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Unix.cs

+                if (toReturn != null)
                {
-                    return new string(buf, 0, realLen);
+                    ArrayPool<char>.Shared.Return(toReturn);


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.

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);

}

}

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.

…ngth on Windows

src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Windows.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Unix.cs

MihaZupan · 2020-04-09T21:53:14Z

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

src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Windows.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Unix.cs

stephentoub · 2020-04-10T02:51:17Z

src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Windows.cs

-                        throw new ArgumentException(SR.Argument_InvalidNormalizationForm, nameof(normalizationForm));
+                        realLength = Interop.Normaliz.NormalizeString(normalizationForm, pInput, strInput.Length, pDest, buffer.Length);
                    }
+                    int lastError = Marshal.GetLastWin32Error();


Is this valid even if realLength > 0?

(I see the above comment now)

stephentoub

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

Reduce allocations in string.Normalize

226e188

MihaZupan added the area-System.Globalization label Apr 9, 2020

MihaZupan added this to the 5.0 milestone Apr 9, 2020

MihaZupan requested review from stephentoub and tarekgh April 9, 2020 18:02

tarekgh reviewed Apr 9, 2020

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Normalization.cs Outdated Show resolved Hide resolved

tarekgh reviewed Apr 9, 2020

View reviewed changes

src/libraries/Common/src/Interop/Unix/System.Globalization.Native/Interop.Normalization.cs Outdated Show resolved Hide resolved