perf(SourceLength): String.Concat -> StringBuilder - #878
Conversation
|
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 |
|
What happens if this commit is added to PR877? Does the perf gain balance out the performance loss from the bugfix? |
|
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 |
|
Anyway, this was purely an optimisation attempt - #877 tries to actually fix bugs in the function, so functionally that's the one to use |
But I could merge this first, and later you can rebase 877? |
|
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 | ```
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:
After: