Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider adding AppendFormattable to StringBuilder #35047

Closed
paulomorgado opened this issue Apr 16, 2020 · 5 comments
Closed

Consider adding AppendFormattable to StringBuilder #35047

paulomorgado opened this issue Apr 16, 2020 · 5 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Milestone

Comments

@paulomorgado
Copy link
Contributor

paulomorgado commented Apr 16, 2020

I see a lot of code like this:

new StringBuilder().Append$"Now = {DateTime.Now}");

(Well, not exactly like this, but you get my point)

The convenience of string interpolation leads to an unnecessary string instantiation.

Adding an AppendFormattable method to StringBuilder (even if just as extension methods) would keep the benefits of string interpolation but without the drawbacks of the intermediate invocations of string.Format.

public static class StringBuilderExtensions
{
    public static StringBuilder AppendFormattable(this StringBuilder builder, FormattableString formattableString)
        => builder.AppendFormattable(null, formattableString);

    public static StringBuilder AppendFormattable(this StringBuilder builder, IFormatProvider formatProvider, FormattableString formattableString)
        => builder.AppendFormat(formatProvider, formattableString.Format, formattableString.GetArguments());
}

Now I can do this:

new StringBuilder().AppendFormattable($"Now = {DateTime.Now}");
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Runtime untriaged New issue has not been triaged by the area owner labels Apr 16, 2020
@stephentoub
Copy link
Member

stephentoub commented Apr 16, 2020

The convenience of string interpolation leads to an unnecessary string instantiation.

In your example, even with the suggested AppendFormattable there's still an object[] allocation and a boxing allocation for the DateTime, plus the additional FormattableString allocation. So it's actually the same number of allocations, albeit potentially for slightly less total memory.

The more efficient way to write this would be:

sb.Append("Now = ").Append(DateTime.Now);

As there's currently no DateTime-based overload of Append, this will still allocate the boxing object, but it won't incur the allocation for the object[] nor for the FormattableString, and it won't incur the formatting overhead associated with parsing the format string and the like. We could choose to optimize Append(object) internally as well to be more efficient for something like DateTime.

I'd prefer to see this addressed with something like #28945 and dotnet/csharplang#2302.

@paulomorgado
Copy link
Contributor Author

I'm not trying to fix the boxing of value types.

I'm just trying to provide something that works today based on the intuition that, in the general case, the cost of the instantiation of a System.Runtime.CompilerServices.FormattableStringFactory+ConcreteFormattableString will be less than the cost of a throw away string.

And the new method name is because the current compiler prefers string to IFormattable as the target type of the conversion of the interpolation.

But, of course, those proposals, when implemented are better.

@stephentoub
Copy link
Member

stephentoub commented Apr 16, 2020

based on the intuition that

You can just measure :)

private StringBuilder _sb = new StringBuilder();
private DateTime _dt = DateTime.Now;

[Benchmark]
public void AppendFormat()
{
    _sb.Clear();
    FormattableString fs = $"Now = {_dt}";
    _sb.AppendFormat(fs.Format, fs.GetArguments());
}

[Benchmark]
public void AppendString()
{
    _sb.Clear();
    string s = $"Now = {_dt}";
    _sb.Append(s);
}

[Benchmark]
public void AppendAppend()
{
    _sb.Clear();
    _sb.Append("Now = ").Append(_dt);
}
Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
AppendFormat 316.0 ns 1.41 ns 1.25 ns 0.0138 - - 88 B
AppendString 305.4 ns 0.72 ns 0.64 ns 0.0162 - - 104 B
AppendAppend 258.2 ns 3.09 ns 2.74 ns 0.0138 - - 88 B

@paulomorgado
Copy link
Contributor Author

paulomorgado commented Apr 16, 2020

I can even measure with something more close to the code I was reviewing. 😄

private readonly StringBuilder sb = new StringBuilder();
private readonly int age = 52;
private readonly float heigh = 1.73f;
private readonly string firstName = "Paulo";
private readonly string lastName = "Morgado";

[Benchmark]
public void AppendFormat()
{
    this.sb.Clear();
    FormattableString fs = $"My name is {this.firstName} {this.lastName}. I'm {this.age} years old and {this.heigh}m tall.";
    this.sb.AppendFormat(fs.Format, fs.GetArguments());
}

[Benchmark]
public void AppendString()
{
    this.sb.Clear();
    string s = $"My name is {this.firstName} {this.lastName}. I'm {this.age} years old and {this.heigh}m tall.";
    this.sb.Append(s);
}

[Benchmark]
public void AppendAppend()
{
    this.sb.Clear();
    this.sb
        .Append("My name is ")
        .Append(this.firstName)
        .Append(" ")
        .Append(this.lastName)
        .Append(". I'm ")
        .Append(this.age)
        .Append(" years old and ")
        .Append(this.heigh)
        .Append("m tall.");
}
Method Mean Error StdDev Median Gen 0 Gen 1 Gen 2 Allocated
AppendFormat 1,764.8 ns 101.53 ns 299.36 ns 1,666.4 ns 0.0496 - - 209 B
AppendString 2,077.2 ns 138.18 ns 398.68 ns 2,026.4 ns 0.0763 - - 321 B
AppendAppend 987.5 ns 29.11 ns 84.44 ns 976.4 ns 0.0172 - - 72 B

By the way, AppendAppend was my first recommendation.

@joperezr joperezr added api-needs-work API needs work before it is approved, it is NOT ready for implementation api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Jul 7, 2020
@joperezr joperezr added this to the Future milestone Jul 7, 2020
@terrajobst terrajobst removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 25, 2021
@stephentoub
Copy link
Member

The StringBuilder.Append{Line} overloads for string interpolation added in .NET 6 address this. Given that, I'm going to close this issue. Thanks.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Projects
None yet
Development

No branches or pull requests

5 participants