ValueTuple: making ToString consistent with System.Tuple

[jcouv]

@stephentoub, I realized the last change (regarding 8th factory method) had un-intended effects on ToString. It no longer behaves like System.Tuple.ToString.
This PR fixes the problem and adds some dedicated testing (comparing ToString outputs between Tuple and ValueTuple.

[stephentoub]

This pattern is a bit unfortunate, as it means that the common case of calling ToString on a ValueTuple of arity 1-7 is being penalized for the less common case of calling ToString on a ValueTuple of arity greater 7.

There are a variety of alternatives that could avoid the extra string allocation here. The most obvious is you could just duplicate the implementation across the two methods, e.g.

public override string ToString() => "(" + Item1 + ", " + Item2 + ")";
string ITupleInternal.ToStringEnd() => Item1 + ", " + Item2 + ")";

If you wanted to avoid such duplication, you could incur some small extra cost just for the ValueTuple of arity greater than 7 by not having ITupleInternal.ToStringEnd at all, and in ValueTuple8's ToString implementation using rest.ToString().Substring(1) instead of rest.ToStringEnd();.

There are of course other alternatives as well.

Regardless of what approach you take, I'd suggest that rather than just doing + ItemN +, you do + ItemN.ToString() +. With the current code, the compiler is going to use the string.Concat overloads that take objects or object[]s. This means that for value type items, they're going to get boxed and then have ToString called on them, rather than just having ToString called on them to begin with. Further, for most of the ToString implementations, the object[] overload is going to be used, so the compiler is going to allocate an object[] for the params, and then the string.Concat implementation is going to allocate a string[] to store the results of the ToStrings before creating the resulting string. If you instead use ToString on each item, you'll use the string.Concat overload that takes strings or string[]s, and you'll avoid both the box per item as well as the object[] allocation.

[jcouv]

I agree it's unfortunate. I'll duplicate the code and add the element-wise ToString as you suggested.

[jcouv]

@stephentoub Updated as you suggested.

[ghost]

nit: string interpolation over string concatenation.

return $"({Item1})";  // no need to call ToString()

(same goes for the rest of the return statements in this file where strings are explicitly concatenated)

[stephentoub]

string interpolation over string concatenation

Why?

// no need to call ToString()

There's no "need" to call ToString() with the concat version, either, but it's good for performance to do so. #8025 (diff)

I'd be ok with a string interpolation version, especially for the shorter tuples where no additional array would need to be allocated, but you'd still want to use ToString to avoid the extra boxing on each item.

[ghost]

@stephentoub, I thought string interpolation outperforms string concatenation. Apparently, it performs rather poorly (3x slower). I created a quick test: https://gist.github.com/jasonwilliams200OK/5395c3ea4c28b5e4673d28c424aed272, with j = $"{j} : {i.ToString()}", it takes about 600ms and while j = j + ":" + i.ToString(); gives ~180ms on my machine.

Is this a known fact that to get better performance, we should use concatenation and to save allocations, use interpolation (at the cost of performance)? And to get best of both worlds, use StringBuilder.

[stephentoub]

I thought string interpolation outperforms string concatenation

It really depends. Let's take a few examples.

Let's say we're using short strings a and b. If you're comparing:
$"({a})" and "(" + a + ")", then you're really comparing string.Format("({0})", a) and string.Concat("(", a, ")"). The latter, which uses the string, string, string overload of string.Concat has very little work to do: the only allocation is the resulting string, and it only needs to get the lengths of each input string, allocate the resulting string, and copy the input strings into it. In contrast, string.Format needs to parse the format string, call ToString on the object argument, etc. It also needs somewhere to store the output it's building up, and for that it uses a StringBuilder. Now, that StringBuilder is taken from the StringBuilderCache, but StringBuilderCache only supports caching StringBuilders up to a particular size, 320 chars IIRC (though it may have changed). So, if a here is very short, the string.Format version likely won't have additional allocations beyond what the string.Concat version does, but if a ends up being long enough to go beyond the caching threshold, you could end up requiring more allocations as the StringBuilder needs to grow.

Now, let's say you're comparing $"({a}, {b})" and "(" + a + ", " + b + ")". The former is the equivalent of string.Format("({0}, {1})", a, b) and the latter string.Concat("(", a, ", ", b, ")"). There is no overload of string.Concat that takes five strings, so you end up using the params string[] overload, causing this to allocate a string[] to hold the input strings (plus a string[] internally that serves as a defensive copy of the input array). The Concat performance though is again very quick, as it again just needs to add up the lengths of each string, allocate a new one, and copy in the inputs. The string.Format one doesn't need that initial string array allocation, because there are only two object arguments being passed to it, not five, and thus it doesn't need to allocate a params object[], but it does still need to do all of the parsing and ToString'ing and whatnot, and it does use the StringBuilderCache and could potentially fall of the cliff and need to allocate for the StringBuilder.

This is all to say that it's a bit easier to predict the performance of the string.Concat case, especially if you don't know how big the input strings will be and whether they'll end up causing allocations for the StringBuilder. But, string interpolation has the nice benefit of leading to very easy to read code, and in general its performance is fine.

[gafter]

String interpolation has quite different (and more complicated) semantics compared to string concatenation. See dotnet/roslyn#9212 (comment)

[jcouv]

CC @dotnet/roslyn-compiler @dsyme as FYI. Increasing the consistency of ValueTuple with Tuple.

[gafter]

Shouldn't this be ")"?

[jcouv]

Yes, thanks. I must be missing a test. I will fix and check coverage.

[jcouv]

@stephentoub After Neal pointed out a bug in ToStringEnd with VT0, I realized I'm missing test coverage. In doing so, I found another one (unchecked cast to ITupleInternal).
Latest commit fixes both of the bugs and adds more tests.

[stephentoub]

Nit: I'd normally write this as:

return result == null ? 8 : 7 + rest.Size;

but it's up to you.

[jcouv]

Done.

[stephentoub]

LGTM. Thanks.

[jcouv]

@stephentoub I pushed a rebased/squashed commit that you can merge when ready.

[jcouv]

Test Innerloop OSX Debug Build and Test please