Skip to content
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

Jump to bottom

Remove check from FillStringChecked and rename it to CopyStringContent #85880

Merged
merged 10 commits into from
May 9, 2023
Merged

Remove check from FillStringChecked and rename it to CopyStringContent #85880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
175b7e8
Remove AO from a couple of SpanHelpers methods
EgorBo May 5, 2023
d22aa95
Merge branch 'main' of github.com:dotnet/runtime
EgorBo May 6, 2023
ec7503a
Address feedback
EgorBo May 7, 2023
be05813
Merge branch 'main' of github.com:dotnet/runtime into opt-string-fill…
EgorBo May 8, 2023
959df9d
Address feedback
EgorBo May 8, 2023
f5919ac
remove [AI]
EgorBo May 8, 2023
8ff9a62
Address feedback
EgorBo May 8, 2023
f368752
Address feedback
EgorBo May 8, 2023
fdbb1ac
Merge branch 'main' of github.com:dotnet/runtime into opt-string-fill…
EgorBo May 8, 2023
49509cd
Switch to int.MaxValue
EgorBo May 8, 2023
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
3 changes: 3 additions & 0 deletions src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4220,4 +4220,7 @@
<data name="Argument_UnexpectedStateForKnownCallback" xml:space="preserve">
<value>An unexpected state object was encountered. This is usually a sign of a bug in async method custom infrastructure, such as a custom awaiter or IValueTaskSource implementation.</value>
</data>
<data name="OutOfMemory_StringTooLong" xml:space="preserve">
<value>The resulting string is too long</value>
</data>
</root>
71 changes: 44 additions & 27 deletions src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,11 @@ internal static class SearchValuesStorage
internal const int StackallocIntBufferSizeLimit = 128;
internal const int StackallocCharBufferSizeLimit = 256;

private static void FillStringChecked(string dest, int destPos, string src)
private static void CopyStringContent(string dest, int destPos, string src)
{
Debug.Assert(dest != null);
Debug.Assert(src != null);
if (src.Length > dest.Length - destPos)
{
throw new IndexOutOfRangeException();
}
Debug.Assert(src.Length <= dest.Length - destPos);

Buffer.Memmove(
destination: ref Unsafe.Add(ref dest._firstChar, destPos),
Expand Down Expand Up @@ -97,7 +94,7 @@ public static string Concat(params object?[] args)

if (totalLength < 0) // Check for a positive overflow
{
throw new OutOfMemoryException();
ThrowHelper.ThrowOutOfMemoryException_StringTooLong();
}
}

Expand All @@ -117,7 +114,7 @@ public static string Concat(params object?[] args)
Debug.Assert(s != null);
Debug.Assert(position <= totalLength - s.Length, "We didn't allocate enough space for the result string!");

FillStringChecked(result, position, s);
CopyStringContent(result, position, s);
position += s.Length;
}

Expand Down Expand Up @@ -255,10 +252,18 @@ public static string Concat(string? str0, string? str1)

int str0Length = str0.Length;

string result = FastAllocateString(str0Length + str1.Length);
int totalLength = str0Length + str1.Length;

// Can't overflow to a positive number so just check < 0
if (totalLength < 0)
{
ThrowHelper.ThrowOutOfMemoryException_StringTooLong();
}

string result = FastAllocateString(totalLength);

FillStringChecked(result, 0, str0);
FillStringChecked(result, str0Length, str1);
CopyStringContent(result, 0, str0);
CopyStringContent(result, str0Length, str1);

return result;
}
Expand All @@ -280,12 +285,18 @@ public static string Concat(string? str0, string? str1, string? str2)
return Concat(str0, str1);
}

int totalLength = str0.Length + str1.Length + str2.Length;
// It can overflow to a positive number so we accumulate the total length as a long.
long totalLength = (long)str0.Length + (long)str1.Length + (long)str2.Length;

string result = FastAllocateString(totalLength);
FillStringChecked(result, 0, str0);
FillStringChecked(result, str0.Length, str1);
FillStringChecked(result, str0.Length + str1.Length, str2);
if (totalLength > int.MaxValue)
{
ThrowHelper.ThrowOutOfMemoryException_StringTooLong();
}

string result = FastAllocateString((int)totalLength);
CopyStringContent(result, 0, str0);
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
CopyStringContent(result, str0.Length, str1);
CopyStringContent(result, str0.Length + str1.Length, str2);

return result;
}
Expand All @@ -312,13 +323,19 @@ public static string Concat(string? str0, string? str1, string? str2, string? st
return Concat(str0, str1, str2);
}

int totalLength = str0.Length + str1.Length + str2.Length + str3.Length;
// It can overflow to a positive number so we accumulate the total length as a long.
long totalLength = (long)str0.Length + (long)str1.Length + (long)str2.Length + (long)str3.Length;

string result = FastAllocateString(totalLength);
FillStringChecked(result, 0, str0);
FillStringChecked(result, str0.Length, str1);
FillStringChecked(result, str0.Length + str1.Length, str2);
FillStringChecked(result, str0.Length + str1.Length + str2.Length, str3);
if (totalLength > int.MaxValue)
{
ThrowHelper.ThrowOutOfMemoryException_StringTooLong();
}

string result = FastAllocateString((int)totalLength);
CopyStringContent(result, 0, str0);
CopyStringContent(result, str0.Length, str1);
CopyStringContent(result, str0.Length + str1.Length, str2);
CopyStringContent(result, str0.Length + str1.Length + str2.Length, str3);

return result;
}
Expand Down Expand Up @@ -447,7 +464,7 @@ public static string Concat(params string?[] values)
// If it's too long, fail, or if it's empty, return an empty string.
if (totalLengthLong > int.MaxValue)
Copy link
Member

@jkotas jkotas May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Is it better to do this check as if (totalLengthLong > int.MaxValue) or as if (totalLength64 != totalLength32) like what we do in other places?

It would be nice to have uniform style.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say whatever reads better? Surpisingly, int.MaxValue shows slightly better codegen:

static void Foo1(int a, int b)
{
    long result64 = (long)a + (long)b;
    int result32 = (int)result64;
    if (result32 != result64)
        throw new OverflowException();
    Console.WriteLine(result32);
}

static void Foo2(int a, int b)
{
    long result = (long)a + (long)b;
    if (result > int.MaxValue)
        throw new OverflowException();
    Console.WriteLine((int)result);
}

https://www.diffchecker.com/JS15izhA/ (same on arm64)

{
throw new OutOfMemoryException();
ThrowHelper.ThrowOutOfMemoryException_StringTooLong();
}
int totalLength = (int)totalLengthLong;
if (totalLength == 0)
Expand All @@ -470,7 +487,7 @@ public static string Concat(params string?[] values)
break;
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
}

FillStringChecked(result, copiedLength, value);
CopyStringContent(result, copiedLength, value);
copiedLength += valueLen;
}
}
Expand Down Expand Up @@ -931,7 +948,7 @@ private static string JoinCore(ReadOnlySpan<char> separator, ReadOnlySpan<string
long totalSeparatorsLength = (long)(values.Length - 1) * separator.Length;
if (totalSeparatorsLength > int.MaxValue)
{
ThrowHelper.ThrowOutOfMemoryException();
ThrowHelper.ThrowOutOfMemoryException_StringTooLong();
}
int totalLength = (int)totalSeparatorsLength;

Expand All @@ -943,7 +960,7 @@ private static string JoinCore(ReadOnlySpan<char> separator, ReadOnlySpan<string
totalLength += value.Length;
if (totalLength < 0) // Check for overflow
{
ThrowHelper.ThrowOutOfMemoryException();
ThrowHelper.ThrowOutOfMemoryException_StringTooLong();
}
}
}
Expand All @@ -970,7 +987,7 @@ private static string JoinCore(ReadOnlySpan<char> separator, ReadOnlySpan<string
}
EgorBo marked this conversation as resolved.
Show resolved Hide resolved

// Fill in the value.
FillStringChecked(result, copiedLength, value);
CopyStringContent(result, copiedLength, value);
copiedLength += valueLen;
}

Expand Down Expand Up @@ -1295,7 +1312,7 @@ private string ReplaceHelper(int oldValueLength, string newValue, ReadOnlySpan<i

long dstLength = this.Length + ((long)(newValue.Length - oldValueLength)) * indices.Length;
if (dstLength > int.MaxValue)
throw new OutOfMemoryException();
ThrowHelper.ThrowOutOfMemoryException_StringTooLong();
string dst = FastAllocateString((int)dstLength);

Span<char> dstSpan = new Span<char>(ref dst._firstChar, dst.Length);
Expand Down
6 changes: 6 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,12 @@ internal static void ThrowOutOfMemoryException()
throw new OutOfMemoryException();
}

[DoesNotReturn]
internal static void ThrowOutOfMemoryException_StringTooLong()
{
throw new OutOfMemoryException(SR.OutOfMemory_StringTooLong);
}

[DoesNotReturn]
internal static void ThrowArgumentException_Argument_IncompatibleArrayType()
{
Expand Down