Solve recently introduce ASCII regression

[adamsitnik]

One of the goals I had in #75012 was to replace the usage of internal helpers with new public API calls.

Initially we wanted to make Ascii.GetIndexOfFirstNonAsciiChar public, but at the end we decided to keep it internal.

Here I am removing the redundant call to Ascii.GetIndexOfFirstNonAsciiChar(ROS) with a call to the helper method that accepts a pointer and the length. What we get is one less method call, one less span creation and pinning.

fixes #80247

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-encoding
See info in area-owners.md if you want to be subscribed.

Issue Details

---

One of the goals I had in #75012 was to replace the usage of internal helpers with new public API calls.

Initially we wanted to make Ascii.GetIndexOfFirstNonAsciiChar public, but at the end we decided to keep it internal.

Here I am removing the redundant call to Ascii.GetIndexOfFirstNonAsciiChar(ROS) with a call to the helper method that accepts a pointer and the length. What we get is one less method call, one less span creation and pinning.

fixes #80247

Author:	adamsitnik
Assignees:	-
Labels:	area-System.Text.Encoding
Milestone:	-

[tarekgh]

LGTM. I assume we have a good test coverage for the changed APIs.

[adamsitnik]

I've applied no merge label as I am about to apply the same pattern in few other places and it would be better to merge of all of them in a single PR.

[stephentoub]

If this overhead resulted in a measurable regression, does that suggest there's something to be changed about the public APIs? Presumably everyone else using them would hit similar issues?

[adamsitnik]

If this overhead resulted in a measurable regression, does that suggest there's something to be changed about the public APIs? Presumably everyone else using them would hit similar issues?

The overhead is quite specific to our needs and is visible only for small inputs. Most of the Encoding methods pin the buffer and then just work with raw pointers:

runtime/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIEncoding.cs

Lines 96 to 99 in e9b9489

	fixed (char* pChars = chars)
	{
	return GetByteCountCommon(pChars + index, count);
	}

runtime/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIEncoding.cs

Lines 114 to 117 in e9b9489

	fixed (char* pChars = chars)
	{
	return GetByteCountCommon(pChars, chars!.Length);
	}

runtime/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIEncoding.cs

Lines 144 to 147 in e9b9489

	fixed (char* charsPtr = &MemoryMarshal.GetReference(chars))
	{
	return GetByteCountCommon(charsPtr, chars.Length);
	}

We introduced Span-accepting methods that are also pinning the memory and then just passing a pointer to the helper. So, the overhead comes from creating a span (from the pointer and length), pinning the memory (again), having one more method call (to our public method) and doing some basic checks again (like checking for empty span). It’s very little, but here it matters.

runtime/src/libraries/System.Private.CoreLib/src/System/Text/ASCIIEncoding.cs

Line 196 in e9b9489

int firstNonAsciiIndex = Ascii.GetIndexOfFirstNonAsciiChar(new ReadOnlySpan<char>(pChars, charsLength));

runtime/src/libraries/System.Private.CoreLib/src/System/Text/Ascii.cs

Lines 25 to 27 in e9b9489

	fixed (byte* pBuffer = &MemoryMarshal.GetReference(buffer))
	{
	nuint idxOfFirstNonAsciiElement = GetIndexOfFirstNonAsciiByte(pBuffer, bufferLength);

I've checked if we could get rid of pinning, but plenty of our ASCII helpers assume that the pointer is fixed and implement optimizations that are searching for the first aligned address:

runtime/src/libraries/System.Private.CoreLib/src/System/Text/Ascii.Utility.cs

Line 318 in e9b9489

// Now adjust the read pointer so that future reads are aligned.

We could switch to using manager pointers and remove the optimizations that try to take advantage of the alignment, but it would be very hard to achieve performance parity.

FWIW I am not happy about making these changes, as it’s as step back s but it seems to be the lesser evil in this particular case.