Reduce allocations during ToDisplayString #69800
Conversation
This method shows as 5.9% of allocations in a profile during a slow completion operation. Of that, 3.1% is attributable to ImmutableArray allocations. ToDisplayString previously would populate ArrayBuilder instances, allocate immutable arrays from them, iterate over the immutable arrays to generate a string, and then throw away the immutable arrays. Instead, we can just skip the ImmutableArrays completely and iterate over ArrayBuilders to generate the string.
ghost
added
Area-Compilers
untriaged
| { | ||
| if (parts is null) | ||
| { | ||
| throw new ArgumentException("parts"); |
There was a problem hiding this comment.
| throw new ArgumentException("parts"); | |
| throw new ArgumentNullException(nameof(parts)); |
There was a problem hiding this comment.
Good catch, changed here and the code it was copied from
| if (parts.Count == 1) | ||
| { | ||
| return parts[0].ToString(); | ||
| } |
There was a problem hiding this comment.
| if (parts.Count == 1) | |
| { | |
| return parts[0].ToString(); | |
| } | |
| if (parts is [var singlePart]) | |
| { | |
| return singlePart.ToString(); | |
| } |
There was a problem hiding this comment.
I'm a bit hesitant to make this change. I'm not of the opinion that it improves readability in this case, and I think it might add a null check in the code.
|
Also does this fix #67067? |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
I didn't know that bug existed, but this should partially address that bug. Since I don't have a bug tracking this, I'll go ahead and resolve that bug once this PR is in, as I think further allocation optimizations in this method would be quite a bit more involved. |
|
@AlekseyTs @333fred ptal |
ToddGrun
force-pushed
the
dev/toddgrun/OptimizeToDisplayString
branch
from
01c2b62 to
91b0e80
Compare
333fred
left a comment
333fred
left a comment
There was a problem hiding this comment.
Generally LGTM, but it looks like there are a couple of comments to address before approving.
Incorporated the nameof suggestion. Passed on the pattern matching suggestion as I'm not (yet) of the opinion that it improves readability. If you feel otherwise, I'm glad to switch it over. |
| } | ||
| finally | ||
| { | ||
| pool.Free(); |
There was a problem hiding this comment.
I thought there was a ToStringAndFree helper for this pattern.
There was a problem hiding this comment.
Yep, there is. Changed this one and the one I copied this from.
AlekseyTs
left a comment
AlekseyTs
left a comment
There was a problem hiding this comment.
Changes under Compilers LGTM (commit 5)
6 participants
This method shows as 14.2% of allocations of the RoslynCodeAnalysisService process in a profile during a slow razor completion operation. Of that, 7.5% is attributable to ImmutableArray allocations. ToDisplayString previously would populate ArrayBuilder instances, allocate immutable arrays from them, iterate over the immutable arrays to generate a string, and then throw away the immutable arrays. Instead, we can just skip the ImmutableArrays completely and iterate over ArrayBuilders to generate the string.
Benchmarks code used to generate the following data is from the 2nd commit.