Optimize using StringBuilder #24755

SergerGood · 2021-04-25T09:52:35Z

No description provided.

ArminShoeibi · 2021-04-25T10:39:12Z

Nice.
https://stackoverflow.com/questions/42721009/performance-of-stringbuilder-append

roji · 2021-04-25T13:27:11Z

@SergerGood any specific reason to do this change? Are you seeing performance differences between appending a char and a string?

@ArminShoeibi your link seems to point to a Java-related question, not .NET.

roji · 2021-04-25T13:28:04Z

src/EFCore.Relational/Metadata/IViewColumn.cs

@@ -44,12 +44,12 @@ string ToDebugString(MetadataDebugStringOptions options = MetadataDebugStringOpt
            var singleLine = (options & MetadataDebugStringOptions.SingleLine) != 0;
            if (singleLine)
            {
-                builder.Append($"ViewColumn: {Table.Name}.");
+                builder.Append("ViewColumn: ").Append(Table.Name).Append('.');


This makes the code less readable, in a codepath that is not perf-sensitive in any way.

In the same place:

efcore/src/EFCore.Relational/Metadata/IViewColumn.cs

Line 50 in ac2bb48

builder.Append(Name).Append(" (");

efcore/src/EFCore.Relational/Metadata/IViewColumn.cs

Line 52 in ac2bb48

builder.Append(StoreType).Append(")");

Still doesn't seem like an improvement to me (note that here there are three things, whereas in your two examples there are two). Can you please revert unless there's a good reason to do this?

I don't know if it's good reason or not.

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042 Intel Core i5-9300H CPU 2.40GHz, 1 CPU, 8 logical and 4 physical cores .NET Core SDK=6.0.100-preview.3.21202.5 [Host] : .NET Core 6.0.0 (CoreCLR 6.0.21.20104, CoreFX 6.0.21.20104), X64 RyuJIT DefaultJob : .NET Core 6.0.0 (CoreCLR 6.0.21.20104, CoreFX 6.0.21.20104), X64 RyuJIT

Method CharactersCount Mean Error StdDev Median Gen 0 Gen 1 Gen 2 Allocated

StringInterpolation 5 32.56 ns 0.679 ns 1.764 ns 32.28 ns 0.0134 - - 56 B

Append 5 14.08 ns 0.309 ns 0.356 ns 14.13 ns - - - -

StringInterpolation 10 35.03 ns 0.714 ns 1.046 ns 35.00 ns 0.0153 - - 64 B

Append 10 14.56 ns 0.903 ns 2.502 ns 13.31 ns - - - -

Benchmark code

[MemoryDiagnoser] public class Program { private StringBuilder _builder; private string _text; [Params(5, 10)] public int CharactersCount { get; set; } [Benchmark] public void StringInterpolation() { _builder.Clear(); _builder.Append($"Column: {_text}."); } [Benchmark] public void Append() { _builder.Clear(); _builder.Append("Column: ").Append(_text).Append('.'); } [GlobalSetup] public void Setup() { _builder = new StringBuilder(); _text = new string('a', CharactersCount); } private static void Main() => BenchmarkRunner.Run<Program>(); }

String interpolation indeed has a perf cost, but this code is completely non-perf-sensitive, so readability is more important than 20ns. Nanosecond-level optimizations which degrade readability really belong only in hot paths where they have a demonstrable impact on end performance.

ArminShoeibi · 2021-04-25T13:30:19Z

@ArminShoeibi your link seems to point to a Java-related question, not .NET.

oh, my bad.

SergerGood · 2021-04-25T17:10:09Z

@SergerGood any specific reason to do this change? Are you seeing performance differences between appending a char and a string?

@roji https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1834

roji

Here's a quick benchmark:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
AppendChar	5.923 ns	0.0093 ns	0.0072 ns	-	-	-	-
AppendString	6.682 ns	0.1175 ns	0.0981 ns	-	-	-	-

So there's roughly a half-nanosecond difference between these two methods, which doesn't seem significant in any of the code paths being touched. Not all quality rules make sense in all contexts...

We can still do this, but please revert the other changes which remove string interpolation - IMO these just make the code less readable for no reason.

Benchmark code

[MemoryDiagnoser]
public class Program
{
    private StringBuilder _builder;

    [Benchmark]
    public void AppendChar()
    {
        _builder.Clear();
        _builder.Append('x');
    }

    [Benchmark]
    public void AppendString()
    {
        _builder.Clear();
        _builder.Append("X");
    }

    [GlobalSetup]
    public void Setup() => _builder = new StringBuilder();

    static void Main(string[] args)
        => BenchmarkRunner.Run<Program>();
}

roji · 2021-04-25T20:14:12Z

src/EFCore.Design/Design/Internal/CSharpHelper.cs

@@ -289,13 +289,13 @@ public virtual string Identifier(string name, ICollection<string>? scope = null)

            if (partStart != name.Length)
            {
-                builder.Append(name.Substring(partStart));
+                builder.Append(name, partStart, name.Length - partStart);


Is there any reason to do this change? The previous code seems simpler.

The reason is that there is a ready-made native better method for this.

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042 Intel Core i5-9300H CPU 2.40GHz, 1 CPU, 8 logical and 4 physical cores .NET Core SDK=6.0.100-preview.3.21202.5 [Host] : .NET Core 6.0.0 (CoreCLR 6.0.21.20104, CoreFX 6.0.21.20104), X64 RyuJIT DefaultJob : .NET Core 6.0.0 (CoreCLR 6.0.21.20104, CoreFX 6.0.21.20104), X64 RyuJIT

Method CharactersCount Mean Error StdDev Median Gen 0 Gen 1 Gen 2 Allocated

StringSubstring 5 23.03 ns 1.147 ns 3.308 ns 21.55 ns 0.0076 - - 32 B

AppendSubstring 5 11.99 ns 0.134 ns 0.119 ns 11.96 ns - - - -

StringSubstring 10 22.92 ns 0.341 ns 0.302 ns 22.81 ns 0.0076 - - 32 B

AppendSubstring 10 11.77 ns 0.216 ns 0.169 ns 11.78 ns - - - -

Benchmark code

[MemoryDiagnoser] public class Program { private StringBuilder _builder; private string _text; private int _partStart; [Params(5, 10)] public int CharactersCount { get; set; } [Benchmark] public void StringSubstring() { _builder.Clear(); _builder.Append(_text.Substring(_partStart)); } [Benchmark] public void AppendSubstring() { _builder.Clear(); _builder.Append(_text, _partStart, _text.Length - _partStart); } [GlobalSetup] public void Setup() { _builder = new StringBuilder(); _text = new string('a', CharactersCount); _partStart = CharactersCount / 2; } private static void Main() => BenchmarkRunner.Run<Program>(); }

#24755 (comment)

I revert using string interpolation.. but there are many examples. And I'm still in favor of uniform approach.

efcore/src/EFCore.Relational/Extensions/RelationalForeignKeyExtensions.cs

Line 134 in ac2bb48

.Append("FK_")

efcore/src/EFCore.Relational/Infrastructure/RelationalOptionsExtension.cs

Line 451 in ac2bb48

builder.Append("CommandTimeout=").Append(Extension._commandTimeout).Append(' ');

What about Substring in the same file?

efcore/src/EFCore.Design/Design/Internal/CSharpHelper.cs

Line 283 in ac2bb48

builder.Append(name, partStart, i - partStart);

We can certainly make things more consistent - but given that nothing here is perf-sensitive, I'd go with what produces the shortest/simplest code, which is string interpolation in most cases (e.g. the CommandTimeout case above).

Accordingly, can you revert this particular change as well? All the rest looks fine.

roji · 2021-04-25T20:14:23Z

src/EFCore.Relational/Metadata/IColumn.cs

@@ -158,12 +158,12 @@ string ToDebugString(MetadataDebugStringOptions options = MetadataDebugStringOpt
            var singleLine = (options & MetadataDebugStringOptions.SingleLine) != 0;
            if (singleLine)
            {
-                builder.Append($"Column: {Table.Name}.");
+                builder.Append("Column: ").Append(Table.Name).Append('.');


Please revert

roji · 2021-04-25T20:14:31Z

src/EFCore.Relational/Metadata/IFunctionColumn.cs

@@ -44,12 +44,12 @@ string ToDebugString(MetadataDebugStringOptions options = MetadataDebugStringOpt
            var singleLine = (options & MetadataDebugStringOptions.SingleLine) != 0;
            if (singleLine)
            {
-                builder.Append($"FunctionColumn: {Table.Name}.");
+                builder.Append("FunctionColumn: ").Append(Table.Name).Append('.');


Please revert

roji · 2021-04-25T20:14:49Z

src/EFCore.Relational/Metadata/ISqlQueryColumn.cs

@@ -44,12 +44,12 @@ string ToDebugString(MetadataDebugStringOptions options = MetadataDebugStringOpt
            var singleLine = (options & MetadataDebugStringOptions.SingleLine) != 0;
            if (singleLine)
            {
-                builder.Append($"SqlQueryColumn: {Table.Name}.");
+                builder.Append("SqlQueryColumn: ").Append(Table.Name).Append('.');


Please revert

roji · 2021-04-25T20:16:46Z

src/EFCore.Relational/Metadata/IViewColumn.cs

@@ -44,12 +44,12 @@ string ToDebugString(MetadataDebugStringOptions options = MetadataDebugStringOpt
            var singleLine = (options & MetadataDebugStringOptions.SingleLine) != 0;
            if (singleLine)
            {
-                builder.Append($"ViewColumn: {Table.Name}.");
+                builder.Append("ViewColumn: ").Append(Table.Name).Append('.');


Still doesn't seem like an improvement to me (note that here there are three things, whereas in your two examples there are two). Can you please revert unless there's a good reason to do this?

roji · 2021-04-25T20:17:57Z

src/Microsoft.Data.Sqlite.Core/SqliteDataReader.cs

-                            .AppendLine($"FROM \"{tableName}\"")
-                            .AppendLine($"WHERE {columnType} != 'null'")
-                            .AppendLine($"GROUP BY {columnType}")
+                            .Append("SELECT ").AppendLine(columnType)


Please revert

ghost · 2021-04-27T17:32:07Z

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-04-27T17:36:03Z

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this

From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at [email protected] to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

Optimize using StringBuilder

9096f28

roji reviewed Apr 25, 2021

View reviewed changes

roji requested changes Apr 25, 2021

View reviewed changes

SergerGood added 2 commits April 27, 2021 19:43

revert

dc23604

revert

31f6c82

roji approved these changes Apr 27, 2021

View reviewed changes

roji added the auto-merge label Apr 27, 2021

ghost merged commit a2b069c into dotnet:main Apr 27, 2021

SergerGood deleted the using-string-builder branch April 27, 2021 19:11

ajcvickers added the community-contribution label May 6, 2021

ajcvickers mentioned this pull request May 6, 2021

Entity Framework Biweekly Status Updates (2021) #23884

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize using StringBuilder #24755

Optimize using StringBuilder #24755

SergerGood commented Apr 25, 2021

ArminShoeibi commented Apr 25, 2021

roji commented Apr 25, 2021

roji Apr 25, 2021

SergerGood Apr 25, 2021

roji Apr 25, 2021

SergerGood Apr 26, 2021

roji Apr 26, 2021 •

edited

Loading

ArminShoeibi commented Apr 25, 2021

SergerGood commented Apr 25, 2021

roji left a comment

roji Apr 25, 2021

SergerGood Apr 26, 2021

roji Apr 26, 2021

SergerGood Apr 27, 2021

roji Apr 27, 2021

SergerGood Apr 27, 2021

roji Apr 27, 2021

roji Apr 25, 2021

roji Apr 25, 2021

roji Apr 25, 2021

roji Apr 25, 2021

roji Apr 25, 2021

ghost commented Apr 27, 2021

ghost commented Apr 27, 2021

From the bot dev team

Method	CharactersCount	Mean	Error	StdDev	Median	Gen 0	Gen 1	Gen 2	Allocated
StringInterpolation	5	32.56 ns	0.679 ns	1.764 ns	32.28 ns	0.0134	-	-	56 B
Append	5	14.08 ns	0.309 ns	0.356 ns	14.13 ns	-	-	-	-
StringInterpolation	10	35.03 ns	0.714 ns	1.046 ns	35.00 ns	0.0153	-	-	64 B
Append	10	14.56 ns	0.903 ns	2.502 ns	13.31 ns	-	-	-	-

Method	CharactersCount	Mean	Error	StdDev	Median	Gen 0	Gen 1	Gen 2	Allocated
StringSubstring	5	23.03 ns	1.147 ns	3.308 ns	21.55 ns	0.0076	-	-	32 B
AppendSubstring	5	11.99 ns	0.134 ns	0.119 ns	11.96 ns	-	-	-	-
StringSubstring	10	22.92 ns	0.341 ns	0.302 ns	22.81 ns	0.0076	-	-	32 B
AppendSubstring	10	11.77 ns	0.216 ns	0.169 ns	11.78 ns	-	-	-	-

Optimize using StringBuilder #24755

Optimize using StringBuilder #24755

Conversation

SergerGood commented Apr 25, 2021

ArminShoeibi commented Apr 25, 2021

roji commented Apr 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

ArminShoeibi commented Apr 25, 2021

SergerGood commented Apr 25, 2021

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 27, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Apr 27, 2021

From the bot dev team

roji Apr 26, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.