Skip to content

perf(SourceLength): String.Concat -> StringBuilder - #878

Open
Numpsy wants to merge 1 commit into
fsprojects:masterfrom
Numpsy:stringbuilder
Open

perf(SourceLength): String.Concat -> StringBuilder#878
Numpsy wants to merge 1 commit into
fsprojects:masterfrom
Numpsy:stringbuilder

Conversation

@Numpsy

@Numpsy Numpsy commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Having another look at this, doing multiple remove operations on a StringBuilders looks to be more efficient than multiple String.Concat calls, even when using Spans

Benchmark before:

| Method         | Mean    | Error    | StdDev   | Gen0       | Gen1      | Gen2      | Allocated |
|--------------- |--------:|---------:|---------:|-----------:|----------:|----------:|----------:|
| LintParsedFile | 1.299 s | 0.0113 s | 0.0100 s | 15000.0000 | 6000.0000 | 2000.0000 | 262.67 MB |

After:

| Method         | Mean    | Error    | StdDev   | Gen0       | Gen1      | Gen2      | Allocated |
|--------------- |--------:|---------:|---------:|-----------:|----------:|----------:|----------:|
| LintParsedFile | 1.264 s | 0.0114 s | 0.0101 s | 14000.0000 | 5000.0000 | 1000.0000 | 233.47 MB |

@Numpsy

Numpsy commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Just another thought when looking at #869 - looks to shave off some more allocations vs. the previous change to use spans but still use String.Concat

@knocte

knocte commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

What happens if this commit is added to PR877? Does the perf gain balance out the performance loss from the bugfix?

@Numpsy

Numpsy commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

I'm just getting a new set of figures on the changes (#879 makes a substantial improvement to them all)

fwiw adding a special case to the 'only a single comment to remove' case to use String.Remove instead of StringBuilder drops the allocations from 234.74MB to 234.66MB by avoiding the StringBuilder allocations, but that might be too small for the extra code to be worth it

@Numpsy

Numpsy commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Anyway, this was purely an optimisation attempt - #877 tries to actually fix bugs in the function, so functionally that's the one to use

@knocte

knocte commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

so functionally that's the one to use

But I could merge this first, and later you can rebase 877?

@Numpsy

Numpsy commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Yes, we could take this one as a performance improvement and then make the other one just a bugfix and tests

Having another look at this, doing multiple remove operations
on a StringBuilders looks to be more efficient than multiple
String.Concat calls, even when using Spans

Benchmark before:
```
| Method         | Mean    | Error    | StdDev   | Gen0       | Gen1      | Gen2      | Allocated |
|--------------- |--------:|---------:|---------:|-----------:|----------:|----------:|----------:|
| LintParsedFile | 1.299 s | 0.0113 s | 0.0100 s | 15000.0000 | 6000.0000 | 2000.0000 | 262.67 MB |

```

After:
```
| Method         | Mean    | Error    | StdDev   | Gen0       | Gen1      | Gen2      | Allocated |
|--------------- |--------:|---------:|---------:|-----------:|----------:|----------:|----------:|
| LintParsedFile | 1.264 s | 0.0114 s | 0.0101 s | 14000.0000 | 5000.0000 | 1000.0000 | 233.47 MB |
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants