24

I was investigating String.Concat : (Reflector)

enter image description here

very strange :

the have the values array ,

they creating a NEW ARRAY for which later they send him to ConcatArray.

Question :

Why they created a new array ? they had values from the first place...

edit

code :

public static string Concat(params string[] values)
{
    if (values == null)
    {
        throw new ArgumentNullException("values");
    }
    int totalLength = 0;
    string[] strArray = new string[values.Length];
    for (int i = 0; i < values.Length; i++)
    {
        string str = values[i];
        strArray[i] = (str == null) ? Empty : str;
        totalLength += strArray[i].Length;
        if (totalLength < 0)
        {
            throw new OutOfMemoryException();
        }
    }
    return ConcatArray(strArray, totalLength);
}
Improve this question
7
  • 1
    Probably because it makes ConcatArray more efficient, not having to special case for nulls. You do know it isn't copying all of the strings right? It's just creating a new array of references to the existing strings. Commented Mar 28, 2012 at 15:14
  • I'm not sure I understand how totalLength could ever be less than zero. What silly thing am I overlooking? Commented Mar 28, 2012 at 19:28
  • 2
    @LarsTech when overflowexception occures - the int is negative.... Commented Mar 28, 2012 at 19:55
  • Wait, how were you inspecting Concat? I've been trying to figure out how to view the code inside built in methods, but all I've found is that I need to use cecil. Commented Mar 29, 2012 at 0:33
  • 1
    @Walkerneo: Take ILSpy or dotPeek to look into the assemblies. Another source would be TypeDescriptor. Commented May 14, 2012 at 9:48

6 Answers 6

Reset to default
34

Well for one thing, it means that the contents of the new array can be trusted to be non-null.... and unchanging.

Without that copying, another thread could modify the original array during the call to ConcatArray, which presumably could throw an exception or even trigger a security bug. With the copying, the input array can be changed at any time - each element will be read exactly once, so there can be no inconsistency. (The result may be a mixture of old and new elements, but you won't end up with memory corruption.)

Suppose ConcatArray is trusted to do bulk copying out of the strings in the array it's passed, without checking for buffer overflow. Then if you change the input array at just the right time, you could end up writing outside the allocated memory. Badness. With this defensive copy, the system can be sure1 that the total length really is the total length.

1 Well, unless reflection is used to change the contents of a string. But that can't be done without fairly high permissions - whereas changing the contents of an array is easy.

Improve this answer
Sign up to request clarification or add additional context in comments.

17 Comments

Another thread could modify the original during the copy too. This seems to me to not be correct reasoning. (I didn't downvote, just didn't upvote.)
@Jason: It doesn't matter though - each element is only read once (note the copy to str rather than repeated access) - so either it will be read before the change or after the change. (Some elements can be read before the change and some after, and that's fine.) The totalLength will definitely still be appropriate for the new array, and all the values will still be guaranteed to be non-null.
Your conjecture is correct; this is to ensure safety if someone is modifying the array on another thread.
@Jon Skeet: I get that, but there's still a race condition that is not being avoided by this.
@Jason: You are right; I should have called out that the "safety" I am talking about is not "thread safety" -- whatever that is -- but rather memory safety. The string code is behind the scenes going to use unsafe code to fill in the new buffer, and we must at all costs avoid corrupting memory. If the result is nonsensical because someone mutated an array that was being used on another thread, that's their problem; at least there is a result and an uncorrupted heap.
|
16

Why did they create a new array?

I can confirm Jon's conjecture; I have the original source code in front of me. The comments indicate that the reason for the copy is because some foolish person could be mutating the array that was passed in on another thread. What could then happen? The calculation of length could say that there will be a hundred bytes of string data in the result, but by the time the copy happens, there could be a million bytes of string data in the array.

That would be bad. The problem is prevented easily by making a copy.

Improve this answer

4 Comments

The problem is mitigated, not prevented; the other thread could still mutate the source array before the copy is completed.
@phoog: Right; the problem that is prevented is memory corruption. The output string can still be completely unrelated to the input strings due to race conditions, but memory will not be corrupted.
Ah, I see, because the code reads the length of the string after it is written to the new array, so the totalLength variable is always consistent with the total length of the strings in the new array.
Perhaps this might be a good time to remind the readers that one does't need to be a C# compiler developer (or even a Microsoft employee) to actually see the original source for most framework assemblies, thanks to the Microsoft Reference Source program.
4

They created a new array to normalize out null entries into String.Empty. This couldn't be done on the provided values array because then they would be modifying the input.

Improve this answer

Comments

2

Inefficient

No, it doesn't matter. That array creation and copy is blazing fast relative to the concatenation, it's only copying references.

It looks like they do it to convert null strings in the input array into String.Empty (they can't do this on values because it would be modifying the input which is a no no), and to detect concatentations that would overflow before actually doing the concatenation (that's what the if(totalLength < 0) test is for). Additionally, they can use totalLength to allocate the memory for the concatenated string up front, which is more efficient.

Improve this answer

2 Comments

It doesn't matter in most cases, but if the array is huge, the memory overhead will not be insignificant, and the likelihood of large object heap fragmentation will be significantly higher.
@phoog: If the array is huge then you are almost certainly making an even huger string, and so you already have bigger problems to worry about than the array copy.
0

Probably to make sure it doesn't change over the lifetime of the method. Which would lead to totalLength not fitting the content of the array anymore.

I suspect that ConcatArray uses some unsafe memory copying, and doesn't recheck the string Lengths again. One could rewrite it to avoid the allocation, but one additional small, short-lived allocation is pretty cheap.

Improve this answer

Comments

0

as far as I can see they do some work with this - guess you don't want to mutate the original one ... remember you want your strings immutable same for the Concat-function of course - don't change a parameter-reference if not stated ....

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.