Avoid unnecessary boxing with String.Concat

[stephentoub]

When string concatenation encounters non-strings (e.g. path + '/', name + someInt32, etc.), it calls overloads of String.Concat that accept objects. These overloads then just call ToString on each of the objects, mapping to the C# spec which states that "any non-string argument is converted to its string representation by invoking the virtual ToString method inherited from type object." When any of the individual items being concatenated is a value type, that object first gets boxed to be passed to String.Concat as an object, only to then have ToString called on the boxed object.

This commit changes the local rewriter for string concatenation to test whether an argument is a value type, and if it is, to call ToString on it directly, rather than first boxing it. This can then affect which overload of Concat is used, as the type of the argument has changed to be String. The primary benefit of this is saving the allocation per value-type item. There are some secondary benefits, as well; for example, as there is a four-string overload of Concat but no four-object overload of Concat, if this optimization is able to force all of the items to be strings, the four-string overload can be used rather than allocating an object array for the inputs and then another string array for the resulting strings (inside of Concat).

(Beyond this optimization, there's the potential for another, which converts literal char arguments to be literal string arguments, avoiding the need for ToString's allocation altogether. But as that doesn't stick to the letter of the spec as I read it, I've not done so.)

There are lots of places where such an optimization is beneficial. Just searching through the Reference Source, there are many examples where code is unnecessarily boxing due to this. Just a few examples:

• http://referencesource.microsoft.com/#mscorlib/system/globalization/datetimeformat.cs,610
• http://referencesource.microsoft.com/#System.Core/Microsoft/Scripting/Actions/CallSite.cs,272
• http://referencesource.microsoft.com/#System/net/System/Net/IPAddress.cs,229
• http://referencesource.microsoft.com/#System/net/System/URI.cs,5036
• http://referencesource.microsoft.com/#mscorlib/system/text/encoding.cs,697
• http://referencesource.microsoft.com/#mscorlib/system/io/path.cs,1170

Roslyn itself bumps up against this, for example:

• roslyn/src/Features/Core/Diagnostics/DiagnosticAnalyzerService.DiagnosticAnalyzersAndStates.cs

  Line 180 in 6f35e33

  return ValueTuple.Create(UserDiagnosticsPrefixTableName + "_" + type + "_" + providerType.AssemblyQualifiedName, GetProviderVersion(location));

• roslyn/src/Features/Core/Completion/CommonCompletionUtilities.cs

  Line 173 in 03e3045

  textContentBuilder.AddText(NonBreakingSpace + text);

• roslyn/src/Samples/CSharp/ImplementNotifyPropertyChanged/Impl/ExpansionChecker.cs

  Line 102 in b648430

  name = baseName + ++index;

• roslyn/src/Features/Core/ExtractMethod/MethodExtractor.cs

  Line 165 in 03e3045

  var methodName = char.IsUpper(name[0]) ? name : char.ToUpper(name[0]) + name.Substring(1);

• roslyn/src/Features/Core/LanguageServices/AnonymousTypeDisplayService/AbstractAnonymousTypeDisplayService.cs

  Line 83 in 47ff630

  return "'" + c;

Etc.

[VladimirReshetnikov]

It looks like a breaking change and a deviation from C# spec in some cases. If the value type is mutable and its method ToString() is mutating, then currently this mutation is not observable because it's made on a boxed copy. With this change it will directly mutate the value, that can have observable effects. This looks like a safe optimization only for known immutable value types like int, Guid, DateTime etc.

[stephentoub]

Good point. The majority of the cases I've seen this occur with are in fact with chars, ints, bytes, and just a handful of other core types, so I think the decrease in value for the change would be fairly negligable. If you agree this is worthwhile to pursue, I'll go ahead and make that change...?

[VladimirReshetnikov]

Yes, I think we should use a whitelist of most common immutable value types. Any breaking changes should be first approved by C# LDM or Roslyn Compat Council.

I'll go and add a test that asserts the current behavior.

[jaredpar]

@VladimirReshetnikov do you consider it a breaking change to call ToString on the immutable value types? Or are you just referring to the current behavior which would call it on any struct?

[jaredpar]

CC @gafter for his feedback.

The change itself looks good modular @VladimirReshetnikov comment about mutations.

[stephentoub]

Thanks, guys. I've updated the PR based on @VladimirReshetnikov 's feedback.

[VladimirReshetnikov]

I think we can safely add enums, Guid, DateTimeOffset, TimeSpan, and nullable versions of all whitelisted types.

[stephentoub]

Do all enums map to SpecialType.System_Enum? I wasn't sure if that referred to the base Enum type or to any enum.

[VladimirReshetnikov]

No, SpecialType.System_Enum indicates the abstract class System.Enum. Enums (concrete value types) have their property TypeKind == TypeKind.Enum

[stephentoub]

Updated based on the additional feedback.

[gafter]

Do you really need this method? If it is a value type and has any SpecialType, then it should be safe. No?

[sharwell]

@gafter That places an undocumented restriction on the future expansion of the SpecialType enumeration. While it's unlikely to ever be a problem, that's certainly a dangerous position to put yourself in when it can be avoided.

[jaredpar]

Agree with @sharwell

Better to find a place down the road where we forgot to do this optimization vs. finding out we accidentally started doing it to a type where it is not safe.

[stephentoub]

Sounds like I should leave it as is for now. Thanks.

[gafter]

Are we really trying to "defend" against the possibility that we'll add a platform special value type that has a mutating ToString() method? That seems really, really unlikely. I'd rather see this method removed and have the simpler test in the caller.

[stephentoub]

@gafter, I don't feel strongly about it. I removed the extension per your feedback.

[pharring]

Nice! 👍

[VladimirReshetnikov]

Consider including TimeSpan, DateTimeOffset, Guid, Version and Nullable<T> for immutable T.

[stephentoub]

It's a good suggestion. I was aiming to keep this fairly lean; is it cheap to look up such arbitrary types? If so, I can do so; otherwise, I'd suggest we leave this as is for now, and it can always be updated later with more types if necessary.

[stephentoub]

Thanks, all. I rebased/squashed to address merge conflict with @VladimirReshetnikov 's additional test so that the Merge pull request button works in the GitHub UI.

[jaredpar]

LGTM as well.

JaredPar from a phone
http://blog.paranoidcoding.com/

---

From: Vladimir Reshetnikov [email protected]
Sent: Wednesday, February 11, 2015 7:56:00 PM
To: dotnet/roslyn
Cc: Jared Parsons
Subject: Re: [roslyn] Avoid unnecessary boxing with String.Concat (#415)

[:+1:]

Reply to this email directly or view it on GitHubhttps://github.com//pull/415#issuecomment-74013669.

[aelij]

While a bit out of scope for Roslyn, I think it might also be beneficial to have a generic version of String.Concat with up to 3-4 type parameters, which would prevent boxing as well. The same idea could be applied to String.Format.

[sharwell]

Does this change result in different output for code like the following?

string s = "Value: " + 3.1 + (Thread.CurrentCulture = {some other culture});

In the past, the 3.1 would be boxed, then the culture would be set, and finally the 3.1 would be converted to a string using the new culture. Following this change, the 3.1 would be converted to a string using the old culture.

[svick]

In expressions like path + '/', I think there is still one unnecessary allocation, even with this change: the one-character string returned by char.ToString().

Since I think such code is very common, would it make sense to add special overloads of string.Concat() just for that (e.g. string.Concat(string, char) and string.Concat(char, string))? Or is there some other way to avoid the allocation (interning?)?

[stephentoub]

@svick: Yes, I allude to that in my original PR description, at least in the case of literal chars. In your mentioned case, I think there's a limit to how many such overloads you can reasonably add to the libraries. That'd be two overloads for the two-parameter version (string/char, char/string), 6 for the three-parameter version (string/string/char, string/char/string, char/string/string, string/char/char, char/string/char/, char/char/string), etc., and that's just for the case of char; similar needs arise for Int32 and so on. @aelij above mentioned adding generic overloads; that would help to avoid the boxing (at the expense of potentially needing many generic instantiations), and is something folks like @KrzysztofCwalina are considering for this and other such cases for the future, but it also wouldn't address the string allocation you allude to (unless type tests were done in the implementation to special case each of the specific cases, which would theoretically be a possibility).

@sharwell: Yes, that would change the formatting; this can be seen with the following:

var dot = CultureInfo.GetCultureInfo("en-US");
var comma = CultureInfo.GetCultureInfo("fr-FR");
Thread.CurrentThread.CurrentCulture = dot;

Console.WriteLine("Value: " + (object)3.1 + ((Thread.CurrentThread.CurrentCulture = comma) != null ? "" : ""));
Thread.CurrentThread.CurrentCulture = dot;
Console.WriteLine("Value: " + 3.1.ToString() + ((Thread.CurrentThread.CurrentCulture = comma) != null ? "" : ""));

prints

Value: 3,1
Value: 3.1

So, yes, there is a potential behavioral change here. The thought of someone changing culture in the middle of string concatenation bothers me even more than the thought of someone mutating a mutable struct in the middle of a ToString call :), but nevertheless it is a behavioral change. @theoy , @pharring, thoughts? Up to you guys what we do here. I know such corner-cases behavioral changes have been taken for Roslyn... thoughts on this one?

[svick]

@stephentoub Yeah, I agree that adding all those overloads would be way too much. But I think that having just the two two-parameter overloads would improve the common case while not adding too much.

and that's just for the case of char; similar needs arise for Int32 and so on

How come? How can you get the characters of Int32 without calling its ToString()?

[stephentoub]

How can you get the characters of Int32 without calling its ToString()?

To avoid allocations you would just need to be able to determine its resulting length, and you can determine the number of base-10 digits in an Int32 with some math, at least for invariant culture (it's possible there's some culture I'm not aware of where this wouldn't hold.)

[pharring]

Goodness, changing the culture as a side-effect of a string concat!
I'm OK with the change in behavior, although it's worth adding a unit test to show that we've thought about it. @VSadov, @theoy what do you think?

To steal an expression from someone else: @sharwell just Nikoved it (in reference to @VladimirReshetnikov, who is the person most likely to come up with bizarre edge cases).

[jaredpar]

Bizarre indeed.

The C# spec seems to support @stephentoub change over the current behavior. It implies the ToString call will happen as a part of the concatenation operation instead of as an implementation detail of String.Concat

The operands are converted to the parameter types of the selected operator, and the type of the result is the return type of the operator.
Otherwise, any non-string argument is converted to its string representation by invoking the virtual ToString method inherited from type object.

It wouldn't be the first time we deviated from the spec though.

[jaredpar]

Why did we remove the method to explicitly check the SpecialType flag? That behavior is much more future safe than the current approach.

[stephentoub]

@jaredpar, @gafter's strong feedback. I'm ok with whichever approach; someone from Roslyn just needs to tell me which direction to go :) I can undo the most recent commit if that's the desired direction.

[sharwell]

The C# spec seems to support @stephentoub change over the current behavior.

I completely agree, and I see I failed to mention this in my reply. The unfortunate part of this is we are now following the spec, but only for a select few types. The way I interpret the spec is we should only ever call the Concat overloads which take string arguments when optimizing string concatenations. The naive method is for each element of the string concatenation:

1. Load the value.
2. If the value is a value type, box the value.
3. callvirt to Object.ToString().

Then the resulting strings get passed to the appropriate overload of String.Concat.

💡 The optimization in this pull request should be restricted to eliminating boxing operations without also changing the order of operations. I recommend first updating the compiler to evaluate string concatenation arguments in the correct order, and then re-evaluating this optimization in that context. Edit: this will allow further optimizations such as the fact that it's always safe to evaluate ToString() on a readonly field without boxing, because the readonly constraint means the value is already being copied to a temporary variable in the current frame.

[gafter]

👍

[VladimirReshetnikov]

@stephentoub @sharwell Sam, this is an amazing example! To preserve the current behavior we would need to evaluate arguments to string.Concat from left to right, then invoke ToString() on those arguments that are immutable value types from left to right, and then invoke string.Concat. It might require some shuffling of values on the CIL evaluation stack. Interestingly, the C# spec does not specify an order in which ToString() methods of operands of a string concatenation operator are invoked. We can imagine that ToString() of one operand changes the current culture, and it affects ToString() of the other operand. Or they can set some global flags, or throw different exceptions. We likely want a test plan for this, and profiling results of real applications demonstrating a significant performance win that would warrant these changes. Any breaking changes need an approval form the Compat Council or LDM.

[VladimirReshetnikov]

@jaredpar Sorry, I do not follow you argument. Do you mean that the current behavior (without proposed changes) deviates from the spec. Can you give a concrete example?

[stephentoub]

@VladimirReshetnikov, it sounds like you're saying this PR should just be closed (at least for now). Is that the consensus?

[VladimirReshetnikov]

I'd suggest to first add more test coverage around string concatenation and investigate if the current behavior differs from the spec.

[stephentoub]

@VladimirReshetnikov, that's fine, but I don't have the time for that right now. If someone else would like to and has the time, great; we can leave this open pending that outcome. If not, and if that's required to move forward with this PR, then we should close it.

[jaredpar]

Potential compromise:

Only invoke this optimization when the arguments are side effect free. Essentially doesn't call into a method or property. It should be completely safe to do the optimization in that case.

[stephentoub]

@jaredpar, thanks, I'll take a look at doing something along that line of thought.

[VladimirReshetnikov]

@jaredpar @sharwell Yes, I see the deviation from the spec. It's the same in the native C# compiler and Roslyn. But, curiously, if a concatenation is part of an expression tree that is compiled and invoked, the order of evaluation is different and it follows the spec.

[ThatRendle]

Maybe C# 7 can add a pure modifier for properties and methods to help with mad corner cases like this?

[stephentoub]

@jaredpar, I spent some time on that idea, got it all coded up, and then realized that it would end up being a significantly larger change to get it working correctly. As noted in some of the comments, the existing rewriter for this is very local, to the point where in these calls we may not actually have the full set of arguments supplied by the developer. I think this would require finding all such sibling expressions, and that's a much more invasive change.

[CyrusNajmabadi]

Maybe i missed it, but why don't we just apply this optimization to things we know are pure? i.e. the standard primitive types like bool, char, and all the numerics? You'd get the allocation savings, and you wouldn't have to worry about any changes in behavior.

[gafter]

@CyrusNajmabadi System.Double is not pure by this definition, since it uses the current culture to decide what character to use for the decimal point.

[stephentoub]

@CyrusNajmabadi, it's already limited as you say. The concern now is that this changes order of operations, and a subsequent expression in the concatenation could change the current culture in a way that would observably change the results.

[sharwell]

@CyrusNajmabadi @gafter "pure" describes an object (or operation) which does not affect other objects. In this case, we are also concerned with the other way around - i.e. objects which cannot be affected by any other object (or operation). While the primitive types are all pure, the ToString() operation for many of them is affected by the current culture of the system.

[sharwell]

@stephentoub Prior to resolving the ordering issue, we should be able to implement this optimization for types which are immutable, pure, and not affected by other code.

Notably:

• bool
• char (and this was one of the motivating types for this optimization)
• IntPtr
• UIntPtr

[stephentoub]

Thanks, @sharwell. I'd considered that and dismissed it in a moment of thinking that it'd be so limited to not be worthwhile, but after reading your comment and thinking about it again, I agree it would make sense for at least char. I'll revise for that.

[stephentoub]

Ok, take 2 :)

I took @sharwell's suggestion and limited the optimization to just the special value types that don't use culture (or any other state external to the value) in ToString, such that no subsequent expression in the Concat could affect the result of ToString. Since char is verified as such, I also added in the optimization to convert a char literal used in the Concat to be a string literal; this saves not only the boxing at run time but also the string allocation at run time, and depending on the shape of the Concat, it can also result in further constant folding. (Please let me know if there's some reason that this optimization should be backed out, and I'll do so.)

[sharwell]

I'm glad you took the opportunity to improve the compile-time treatment of char literals as string literals here. I've watched many commits over the past few weeks do this by hand, and we'll finally be able to stop worrying about that. Further comments on the diff...

[sharwell]

💡 Since the current behavior is inconsistent with the C# spec, you should include a note here that the attention to preserving this behavior is for strict preservation of the backwards-compatible behavior. Also, I still think we should wait for the "compatibility committee" (or whoever) to review the situation and determine whether or not we should change the Roslyn behavior to actually follow the spec.

❓ How long will it take for the compatibility evaluation to be complete?

[gafter]

The compatibility council is not currently considering this issue because there is no proposal to break compatibility. If such a proposal is made it would be evaluated.

[stephentoub]

I would like to proceed with the current, more-limited optimization, to get something in place now. If we want to submit a follow-up proposal to break compat by expanding the scope, that'll be a relatively trivial PR.

[sharwell]

@gafter Does #522 do the trick?

[stephentoub]

@sharwell, I cleaned up the comment you cited a bit.

@gafter, I realized that the changes I'd made to the various 2-3-many arg rewriters were unnecessary, and I could do the change in a single place in the entry rewriter. This not only simplified the change, it also helped to improve constant folding in some cases and alleviated an array allocation in the many-arg rewriter.

I've pushed the updated commit.

I'm happy with this if others are. @gafter? @jaredpar? (@sharwell's official proposal to allow for more optimizations can then be considered separately.)

[gafter]

👍

[jaredpar]

👍

[stephentoub]

Thanks, all.