From 142f5d6f4e842fccb986ccb07656425d155d980b Mon Sep 17 00:00:00 2001 From: stephentoub Date: Sun, 1 May 2016 07:36:16 -0400 Subject: [PATCH] Port https://github.com/dotnet/coreclr/pull/4559 to corert Avoid defensive copy in String.Concat(string[]) --- .../src/System/String.cs | 79 +++++++++++++------ 1 file changed, 56 insertions(+), 23 deletions(-) diff --git a/src/System.Private.CoreLib/src/System/String.cs b/src/System.Private.CoreLib/src/System/String.cs index 4c120fc5278..6516210db6f 100644 --- a/src/System.Private.CoreLib/src/System/String.cs +++ b/src/System.Private.CoreLib/src/System/String.cs @@ -3038,7 +3038,16 @@ public static String Concat(params Object[] args) throw new OutOfMemoryException(); } } - return ConcatArray(sArgs, totalLength); + + string result = FastAllocateString(totalLength); + int currPos = 0; + for (int i = 0; i < sArgs.Length; i++) + { + FillStringChecked(result, currPos, sArgs[i]); + currPos += sArgs[i].Length; + } + + return result; } public static String Concat(IEnumerable values) @@ -3164,42 +3173,66 @@ public static String Concat(String str0, String str1, String str2, String str3) return result; } - private static String ConcatArray(String[] values, int totalLength) + public static String Concat(params String[] values) { - String result = FastAllocateString(totalLength); - int currPos = 0; + if (values == null) + throw new ArgumentNullException("values"); + + // It's possible that the input values array could be changed concurrently on another + // thread, such that we can't trust that each read of values[i] will be equivalent. + // Worst case, we can make a defensive copy of the array and use that, but we first + // optimistically try the allocation and copies assuming that the array isn't changing, + // which represents the 99.999% case, in particular since string.Concat is used for + // string concatenation by the languages, with the input array being a params array. + // Sum the lengths of all input strings + long totalLengthLong = 0; for (int i = 0; i < values.Length; i++) { - FillStringChecked(result, currPos, values[i]); - currPos += values[i].Length; + string value = values[i]; + if (value != null) + { + totalLengthLong += value.Length; + } } - return result; - } - - public static String Concat(params String[] values) - { - if (values == null) - throw new ArgumentNullException("values"); - int totalLength = 0; - - // Always make a copy to prevent changing the array on another thread. - String[] internalValues = new String[values.Length]; + // If it's too long, fail, or if it's empty, return an empty string. + if (totalLengthLong > int.MaxValue) + { + throw new OutOfMemoryException(); + } + int totalLength = (int)totalLengthLong; + if (totalLength == 0) + { + return string.Empty; + } + // Allocate a new string and copy each input string into it + string result = FastAllocateString(totalLength); + int copiedLength = 0; for (int i = 0; i < values.Length; i++) { string value = values[i]; - internalValues[i] = ((value == null) ? (String.Empty) : (value)); - totalLength += internalValues[i].Length; - // check for overflow - if (totalLength < 0) + if (!string.IsNullOrEmpty(value)) { - throw new OutOfMemoryException(); + int valueLen = value.Length; + if (valueLen > totalLength - copiedLength) + { + copiedLength = -1; + break; + } + + FillStringChecked(result, copiedLength, value); + copiedLength += valueLen; } } - return ConcatArray(internalValues, totalLength); + // If we copied exactly the right amount, return the new string. Otherwise, + // something changed concurrently to mutate the input array: fall back to + // doing the concatenation again, but this time with a defensive copy. This + // fall back should be extremely rare. + return copiedLength == totalLength ? result : Concat((string[])values.Clone()); + } IEnumerator IEnumerable.GetEnumerator()