Skip to content

Refactor fixed statement logic in System.Text #118352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xtqqczze wants to merge 3 commits into from
Closed

Refactor fixed statement logic in System.Text #118352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a832153
Use `string.GetPinnableReference` in fixed where string is not null
xtqqczze Aug 4, 2025
f40ff49
Use `MemoryMarshal.GetArrayDataReference` in fixed where array is not…
xtqqczze Aug 4, 2025
ec6644a
Merge branch 'main' into string-GetPinnableReference-SystemText
xtqqczze Aug 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions src/libraries/System.Private.CoreLib/src/System/Text/ASCIIEncoding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public override unsafe int GetByteCount(char[] chars, int index, int count)
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.chars, ExceptionResource.ArgumentOutOfRange_IndexCountBuffer);
}

fixed (char* pChars = chars)
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
Copy link
Member

@tannergooding tannergooding Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're trying to reduce the amount of unsafe logic in the core libraries, these types of micro-optimizations aren't really something I think we want to take.

It'd rather be better for the JIT to be able to avoid or elide any additional cost here from the GetPinnableReference logic in the case it knows chars is non-null.

Copy link
Member

@tannergooding tannergooding Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC. @EgorBo, @jkotas

Copy link
Member

@jkotas jkotas Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unsafe code both before and after this change. We use MemoryMarshal.GetArrayDataReference and GetPinnableReference micro-optimizations in many places, so I do not see a problem with using it in a few more places.

It'd rather be better for the JIT to be able to avoid or elide any additional cost here from the GetPinnableReference logic in the case it knows chars is non-null.

The JIT does that where possible. "Avoid handling the null case when the variable is known not be null." is not accurate description for majority of the codediff deltas. (There are a few places where it is impossible for the JIT to prove that the null case is unreachable, but these are just a small part of the delta.)

Majority of the codediff deltas are caused by pinning byref instead of object. Before this change, the code pinned object that is a bit more code and a bit friendlier to the GC. After this change, the code pins byref that is a bit less code and a bit less friendly to the GC.

Copy link
Member

@jkotas jkotas Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see a problem with using it in a few more places.

Having said that, I agree that these micro-optimizations add clutter, and the benefit is non-measurable in most cases.

Copy link
Member

@tarekgh tarekgh Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the feedback, I’m closing this PR until it’s shown to make a noticeable performance difference.

{
return GetByteCountCommon(pChars + index, count);
}
Expand All @@ -111,7 +111,7 @@ public override unsafe int GetByteCount(string chars)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.chars);
}

fixed (char* pChars = chars)
fixed (char* pChars = &chars.GetPinnableReference())
{
return GetByteCountCommon(pChars, chars!.Length);
}
Expand Down Expand Up @@ -232,8 +232,8 @@ public override unsafe int GetBytes(string chars, int charIndex, int charCount,
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.byteIndex, ExceptionResource.ArgumentOutOfRange_IndexMustBeLessOrEqual);
}

fixed (char* pChars = chars)
fixed (byte* pBytes = bytes)
fixed (char* pChars = &chars.GetPinnableReference())
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
return GetBytesCommon(pChars + charIndex, charCount, pBytes + byteIndex, bytes.Length - byteIndex);
}
Expand Down Expand Up @@ -280,8 +280,8 @@ public override unsafe int GetBytes(char[] chars, int charIndex, int charCount,
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.byteIndex, ExceptionResource.ArgumentOutOfRange_IndexMustBeLessOrEqual);
}

fixed (char* pChars = chars)
fixed (byte* pBytes = bytes)
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
return GetBytesCommon(pChars + charIndex, charCount, pBytes + byteIndex, bytes.Length - byteIndex);
}
Expand Down Expand Up @@ -453,7 +453,7 @@ public override unsafe int GetCharCount(byte[] bytes, int index, int count)
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.bytes, ExceptionResource.ArgumentOutOfRange_IndexCountBuffer);
}

fixed (byte* pBytes = bytes)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
return GetCharCountCommon(pBytes + index, count);
}
Expand Down Expand Up @@ -571,8 +571,8 @@ public override unsafe int GetChars(byte[] bytes, int byteIndex, int byteCount,
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.charIndex, ExceptionResource.ArgumentOutOfRange_IndexMustBeLessOrEqual);
}

fixed (byte* pBytes = bytes)
fixed (char* pChars = chars)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
{
return GetCharsCommon(pBytes + byteIndex, byteCount, pChars + charIndex, chars.Length - charIndex);
}
Expand Down Expand Up @@ -747,7 +747,7 @@ public override unsafe string GetString(byte[] bytes, int byteIndex, int byteCou
if (byteCount == 0)
return string.Empty;

fixed (byte* pBytes = bytes)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
return string.CreateStringFromEncoding(pBytes + byteIndex, byteCount, this);
}
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Private.CoreLib/src/System/Text/Encoding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ public int GetByteCount(string s, int index, int count)

unsafe
{
fixed (char* pChar = s)
fixed (char* pChar = &s.GetPinnableReference())
{
return GetByteCount(pChar + index, count);
}
Expand Down Expand Up @@ -644,14 +644,14 @@ public byte[] GetBytes(string s, int index, int count)

unsafe
{
fixed (char* pChar = s)
fixed (char* pChar = &s.GetPinnableReference())
{
int byteCount = GetByteCount(pChar + index, count);
if (byteCount == 0)
return Array.Empty<byte>();

byte[] bytes = new byte[byteCount];
fixed (byte* pBytes = &bytes[0])
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
int bytesReceived = GetBytes(pChar + index, count, pBytes, byteCount);
Debug.Assert(byteCount == bytesReceived);
Expand Down
32 changes: 16 additions & 16 deletions src/libraries/System.Private.CoreLib/src/System/Text/Latin1Encoding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public override unsafe int GetByteCount(char[] chars, int index, int count)
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.chars, ExceptionResource.ArgumentOutOfRange_IndexCountBuffer);
}

fixed (char* pChars = chars)
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
{
return GetByteCountCommon(pChars + index, count);
}
Expand All @@ -93,7 +93,7 @@ public override unsafe int GetByteCount(string s)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}

fixed (char* pChars = s)
fixed (char* pChars = &s.GetPinnableReference())
{
return GetByteCountCommon(pChars, s.Length);
}
Expand Down Expand Up @@ -216,8 +216,8 @@ public override unsafe int GetBytes(char[] chars, int charIndex, int charCount,
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.byteIndex, ExceptionResource.ArgumentOutOfRange_IndexMustBeLessOrEqual);
}

fixed (char* pChars = chars)
fixed (byte* pBytes = bytes)
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
return GetBytesCommon(pChars + charIndex, charCount, pBytes + byteIndex, bytes.Length - byteIndex);
}
Expand Down Expand Up @@ -278,8 +278,8 @@ public override unsafe int GetBytes(string s, int charIndex, int charCount, byte
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.byteIndex, ExceptionResource.ArgumentOutOfRange_IndexMustBeLessOrEqual);
}

fixed (char* pChars = s)
fixed (byte* pBytes = bytes)
fixed (char* pChars = &s.GetPinnableReference())
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
return GetBytesCommon(pChars + charIndex, charCount, pBytes + byteIndex, bytes.Length - byteIndex);
}
Expand Down Expand Up @@ -442,8 +442,8 @@ public override unsafe char[] GetChars(byte[] bytes)

char[] chars = new char[bytes.Length];

fixed (byte* pBytes = bytes)
fixed (char* pChars = chars)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
{
GetCharsCommon(pBytes, bytes.Length, pChars, chars.Length);
}
Expand Down Expand Up @@ -477,8 +477,8 @@ public override unsafe int GetChars(byte[] bytes, int byteIndex, int byteCount,
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.charIndex, ExceptionResource.ArgumentOutOfRange_IndexMustBeLessOrEqual);
}

fixed (byte* pBytes = bytes)
fixed (char* pChars = chars)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
{
return GetCharsCommon(pBytes + byteIndex, byteCount, pChars + charIndex, chars.Length - charIndex);
}
Expand Down Expand Up @@ -509,8 +509,8 @@ public override unsafe char[] GetChars(byte[] bytes, int index, int count)

char[] chars = new char[count];

fixed (byte* pBytes = bytes)
fixed (char* pChars = chars)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
{
GetCharsCommon(pBytes + index, count, pChars, chars.Length);
}
Expand Down Expand Up @@ -554,8 +554,8 @@ public override unsafe string GetString(byte[] bytes)
}

string result = string.FastAllocateString(bytes.Length);
fixed (byte* pBytes = bytes)
fixed (char* pChars = result)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
fixed (char* pChars = &result.GetPinnableReference())
{
GetCharsCommon(pBytes, bytes.Length, pChars, result.Length);
}
Expand Down Expand Up @@ -584,8 +584,8 @@ public override unsafe string GetString(byte[] bytes, int index, int count)
}

string result = string.FastAllocateString(count);
fixed (byte* pBytes = bytes)
fixed (char* pChars = result)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
fixed (char* pChars = &result.GetPinnableReference())
{
GetCharsCommon(pBytes + index, count, pChars, count);
}
Expand Down
14 changes: 7 additions & 7 deletions src/libraries/System.Private.CoreLib/src/System/Text/UTF32Encoding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public override unsafe int GetByteCount(char[] chars, int index, int count)
return 0;

// Just call the pointer version
fixed (char* pChars = chars)
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
return GetByteCount(pChars + index, count, null);
}

Expand All @@ -121,7 +121,7 @@ public override unsafe int GetByteCount(string s)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}

fixed (char* pChars = s)
fixed (char* pChars = &s.GetPinnableReference())
return GetByteCount(pChars, s.Length, null);
}

Expand Down Expand Up @@ -162,7 +162,7 @@ public override unsafe int GetBytes(string s, int charIndex, int charCount,

int byteCount = bytes.Length - byteIndex;

fixed (char* pChars = s)
fixed (char* pChars = &s.GetPinnableReference())
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
return GetBytes(pChars + charIndex, charCount, pBytes + byteIndex, byteCount, null);
Expand Down Expand Up @@ -205,7 +205,7 @@ public override unsafe int GetBytes(char[] chars, int charIndex, int charCount,
// Just call pointer version
int byteCount = bytes.Length - byteIndex;

fixed (char* pChars = chars)
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
// Remember that byteCount is # to decode, not size of array.
Expand Down Expand Up @@ -252,7 +252,7 @@ public override unsafe int GetCharCount(byte[] bytes, int index, int count)
return 0;

// Just call pointer version
fixed (byte* pBytes = bytes)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
return GetCharCount(pBytes + index, count, null);
}

Expand Down Expand Up @@ -297,7 +297,7 @@ public override unsafe int GetChars(byte[] bytes, int byteIndex, int byteCount,
// Just call pointer version
int charCount = chars.Length - charIndex;

fixed (byte* pBytes = bytes)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
{
// Remember that charCount is # to decode, not size of array
Expand Down Expand Up @@ -342,7 +342,7 @@ public override unsafe string GetString(byte[] bytes, int index, int count)
// Avoid problems with empty input buffer
if (count == 0) return string.Empty;

fixed (byte* pBytes = bytes)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
return string.CreateStringFromEncoding(
pBytes + index, count, this);
}
Expand Down
14 changes: 7 additions & 7 deletions src/libraries/System.Private.CoreLib/src/System/Text/UTF7Encoding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public override unsafe int GetByteCount(char[] chars, int index, int count)
return 0;

// Just call the pointer version
fixed (char* pChars = chars)
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
return GetByteCount(pChars + index, count, null);
}

Expand All @@ -157,7 +157,7 @@ public override unsafe int GetByteCount(string s)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}

fixed (char* pChars = s)
fixed (char* pChars = &s.GetPinnableReference())
return GetByteCount(pChars, s.Length, null);
}

Expand Down Expand Up @@ -198,7 +198,7 @@ public override unsafe int GetBytes(string s, int charIndex, int charCount,

int byteCount = bytes.Length - byteIndex;

fixed (char* pChars = s)
fixed (char* pChars = &s.GetPinnableReference())
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
return GetBytes(pChars + charIndex, charCount, pBytes + byteIndex, byteCount, null);
Expand Down Expand Up @@ -241,7 +241,7 @@ public override unsafe int GetBytes(char[] chars, int charIndex, int charCount,
// Just call pointer version
int byteCount = bytes.Length - byteIndex;

fixed (char* pChars = chars)
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
{
// Remember that byteCount is # to decode, not size of array.
Expand Down Expand Up @@ -288,7 +288,7 @@ public override unsafe int GetCharCount(byte[] bytes, int index, int count)
return 0;

// Just call pointer version
fixed (byte* pBytes = bytes)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
return GetCharCount(pBytes + index, count, null);
}

Expand Down Expand Up @@ -333,7 +333,7 @@ public override unsafe int GetChars(byte[] bytes, int byteIndex, int byteCount,
// Just call pointer version
int charCount = chars.Length - charIndex;

fixed (byte* pBytes = bytes)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars))
{
// Remember that charCount is # to decode, not size of array
Expand Down Expand Up @@ -378,7 +378,7 @@ public override unsafe string GetString(byte[] bytes, int index, int count)
// Avoid problems with empty input buffer
if (count == 0) return string.Empty;

fixed (byte* pBytes = bytes)
fixed (byte* pBytes = &MemoryMarshal.GetArrayDataReference(bytes))
return string.CreateStringFromEncoding(
pBytes + index, count, this);
}
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Text/UTF8Encoding.Sealed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ private unsafe byte[] GetBytesForSmallInput(string s)
int sourceLength = s.Length; // hoist this to avoid having the JIT auto-insert null checks
int bytesWritten;

fixed (char* pSource = s)
fixed (char* pSource = &s.GetPinnableReference())
{
bytesWritten = GetBytesCommon(pSource, sourceLength, pDestination, MaxSmallInputElementCount * MaxUtf8BytesPerChar);
Debug.Assert(0 <= bytesWritten && bytesWritten <= s.Length * MaxUtf8BytesPerChar);
Expand Down Expand Up @@ -140,7 +140,7 @@ private unsafe string GetStringForSmallInput(byte[] bytes)
int sourceLength = bytes.Length; // hoist this to avoid having the JIT auto-insert null checks
int charsWritten;

fixed (byte* pSource = bytes)
fixed (byte* pSource = &MemoryMarshal.GetArrayDataReference(bytes))
{
charsWritten = GetCharsCommon(pSource, sourceLength, pDestination, MaxSmallInputElementCount);
Debug.Assert(0 <= charsWritten && charsWritten <= sourceLength); // should never have more output chars than input bytes
Expand Down
Loading
Loading