Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Add StringBuilder Span-based APIs #13163

Merged
merged 3 commits into from
Aug 8, 2017
Merged

Add StringBuilder Span-based APIs #13163

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
997f4e8
Add StringBuilder Span-based APIs
kevingosse Aug 1, 2017
e86c365
Factorized ThreadSafeCopy methods
kevingosse Aug 2, 2017
1e41719
Fix possible overflow
kevingosse Aug 3, 2017
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
73 changes: 58 additions & 15 deletions src/mscorlib/shared/System/Text/StringBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,11 +822,6 @@ public void CopyTo(int sourceIndex, char[] destination, int destinationIndex, in
throw new ArgumentNullException(nameof(destination));
}

if (count < 0)
{
throw new ArgumentOutOfRangeException(nameof(count), SR.Arg_NegativeArgCount);
}
Copy link
Member

Choose a reason for hiding this comment

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

Technically this a breaking change, as if multiple invalid inputs were provided, it'll change the order in which we check and thus will change what exceptions are thrown. That said, I personally think that's an acceptable level of break, but others should weigh in... @weshaggard?

Copy link
Member

Choose a reason for hiding this comment

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

I think the change is acceptable as well.


if (destinationIndex < 0)
{
throw new ArgumentOutOfRangeException(nameof(destinationIndex), SR.Format(SR.ArgumentOutOfRange_MustBeNonNegNum, nameof(destinationIndex)));
Expand All @@ -836,6 +831,17 @@ public void CopyTo(int sourceIndex, char[] destination, int destinationIndex, in
{
throw new ArgumentException(SR.ArgumentOutOfRange_OffsetOut);
}
Contract.EndContractBlock();

CopyTo(sourceIndex, new Span<char>(destination, destinationIndex), count);
}

public void CopyTo(int sourceIndex, Span<char> destination, int count)
{
if (count < 0)
{
throw new ArgumentOutOfRangeException(nameof(count), SR.Arg_NegativeArgCount);
}

if ((uint)sourceIndex > (uint)Length)
{
Expand All @@ -852,7 +858,7 @@ public void CopyTo(int sourceIndex, char[] destination, int destinationIndex, in

StringBuilder chunk = this;
int sourceEndIndex = sourceIndex + count;
int curDestIndex = destinationIndex + count;
int curDestIndex = count;
while (count > 0)
{
int chunkEndIndex = sourceEndIndex - chunk.m_ChunkOffset;
Expand Down Expand Up @@ -1033,6 +1039,21 @@ public StringBuilder Append(char[] value)
return this;
}

public StringBuilder Append(ReadOnlySpan<char> value)
{
if (value.Length > 0)
{
unsafe
{
fixed (char* valueChars = &value.DangerousGetPinnableReference())
Copy link

Choose a reason for hiding this comment

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

If possible, I'd prefer to see this flipped -- that is it would be preferable to have the pointer overload create a span and pass that throughout the class. That realizes part of the purpose of spans -- avoiding memory corruption while still being fast. If you're not comfortable doing that in this change, I do understand.

Copy link
Member

Choose a reason for hiding this comment

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

Calls string.wstrcpy though which wants pointers; would need to use equivalent span function in string to that?

Copy link

Choose a reason for hiding this comment

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

Since there's not currently interop support for Span (@yizhang82, any plans?), I agree there are places where pointers are still necessary. However, if we minimize those places, it mitigates cases like the int overflows you pointed out.

Copy link
Member

@benaadams benaadams Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Could add an internal wstrcpy that takes Span and call that instead?

i.e. have the pointers in string rather than elsewhere

/cc @stephentoub

Copy link
Member

@stephentoub stephentoub Aug 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Let's keep such things separate. There are two distinct tracks for span in coreclr/corefx:

  • Using it internally to clean up duplicated code, to make existing unsafe code safer, to make it easier to stack allocate in places we're currently heap allocating, etc.
  • Adding new APIs that accept span

I'd like to largely keep those separate. If it's easy to achieve the former while doing the latter, great, or if the best way to implement the API is by changing existing implementation, ok, but doing so also makes it more likely we'll incur performance regressions, especially at this early stage where we haven't shaken out all of the possible perf issues with span. Any such changes need to be measured and validated rigorously.

{
Append(valueChars, value.Length);
}
}
}
return this;
}

#region AppendJoin

public unsafe StringBuilder AppendJoin(string separator, params object[] values)
Expand Down Expand Up @@ -1263,6 +1284,27 @@ public StringBuilder Insert(int index, char[] value, int startIndex, int charCou

public StringBuilder Insert(int index, Object value) => (value == null) ? this : Insert(index, value.ToString(), 1);

public StringBuilder Insert(int index, ReadOnlySpan<char> value)
{
Contract.Ensures(Contract.Result<StringBuilder>() != null);
Contract.EndContractBlock();

if ((uint)index > (uint)Length)
{
throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_Index);
}

if (value.Length > 0)
{
unsafe
{
fixed (char* sourcePtr = &value.DangerousGetPinnableReference())
Insert(index, sourcePtr, value.Length);
}
}
return this;
}

public StringBuilder AppendFormat(String format, Object arg0) => AppendFormatHelper(null, format, new ParamsArray(arg0));

public StringBuilder AppendFormat(String format, Object arg0, Object arg1) => AppendFormatHelper(null, format, new ParamsArray(arg0, arg1));
Expand Down Expand Up @@ -2001,22 +2043,23 @@ private static unsafe void ThreadSafeCopy(char* sourcePtr, char[] destination, i
}
}

private static void ThreadSafeCopy(char[] source, int sourceIndex, char[] destination, int destinationIndex, int count)
private static unsafe void ThreadSafeCopy(char[] source, int sourceIndex, Span<char> destination, int destinationIndex, int count)
{
if (count > 0)
{
if ((uint)sourceIndex <= (uint)source.Length && (sourceIndex + count) <= source.Length)
if ((uint)sourceIndex > (uint)source.Length || (sourceIndex + count) > source.Length)
Copy link
Member

Choose a reason for hiding this comment

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

int overflow risk

(sourceIndex + count) > source.Length => count > (source.Length - sourceIndex)

{
unsafe
{
fixed (char* sourcePtr = &source[sourceIndex])
ThreadSafeCopy(sourcePtr, destination, destinationIndex, count);
}
throw new ArgumentOutOfRangeException(nameof(sourceIndex), SR.ArgumentOutOfRange_Index);
}
else

if ((uint)destinationIndex > (uint)destination.Length || (destinationIndex + count) > destination.Length)
Copy link
Member

@benaadams benaadams Aug 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

int overflow risk?

(destinationIndex + count) > destination.Length) => count > (destination.Length - destinationIndex)

Copy link
Author

Choose a reason for hiding this comment

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

FWIW, the same condition exists on the other ThreadSafeCopy overload (that I didn't touch), line 2034.
Still, I agree on the risk. Gonna do some further testing.

Copy link
Member

Choose a reason for hiding this comment

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

source.Length - destinationIndex

@benaadams, copy and paste error?

Copy link
Member

Choose a reason for hiding this comment

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

yeah :)

{
throw new ArgumentOutOfRangeException(nameof(sourceIndex), SR.ArgumentOutOfRange_Index);
throw new ArgumentOutOfRangeException(nameof(destinationIndex), SR.ArgumentOutOfRange_Index);
}

fixed (char* sourcePtr = &source[sourceIndex])
fixed (char* destinationPtr = &destination.DangerousGetPinnableReference())
string.wstrcpy(destinationPtr + destinationIndex, sourcePtr, count);
}
}

Expand Down