Cache indent strings in CodeWriter#12366
Conversation
There are typically only a few different sizes requested, and useTabs/tabSize *very* rarely change. Locally, I only saw eight unique combinations of these values when opening and typing in orchardcore. ComputeIndent accounts for 11.0 MB (0.4%) of total allocations and 27 ms in in the ScrollingAndTypingInCohosting speedometer test. With this change, those numbers go to 0.0 MB (0.0%) and 13 ms respectively. Test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/680751
.../Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.IndentCache.cs
Outdated
Show resolved
Hide resolved
DustinCampbell
left a comment
DustinCampbell
left a comment
There was a problem hiding this comment.
Nice work! I think more can be done to reduce allocations here though.
.../Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.IndentCache.cs
Outdated
Show resolved
Hide resolved
.../Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.IndentCache.cs
Outdated
Show resolved
Hide resolved
.../Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.IndentCache.cs
Outdated
Show resolved
Hide resolved
.../Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.IndentCache.cs
Outdated
Show resolved
Hide resolved
| s_tabs.Span[..tabCount].CopyTo(destination); | ||
| s_spaces.Span[..spaceCount].CopyTo(destination[tabCount..]); |
There was a problem hiding this comment.
I think this will probably break on existing code somewhere. These are going to throw if tabCount if greater than 64 or spaceCount is greater than 128. That's what the while loops and Math.Min(...) calls handled in my suggested implementation.
There was a problem hiding this comment.
Nice catch! I think commit 3 should address that. Tests added. If you find a bug in it, I'll concede.
…either tabs/spaces counts exceeded corresponding cached string length.
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
Well this changed a lot while I was asleep. Pretty cool!
| var tabs = SliceOrCreate(tabCount, s_tabs); | ||
| var spaces = SliceOrCreate(spaceCount, s_spaces); |
There was a problem hiding this comment.
However unlikely, it doesn't feel right to potentially allocate strings inside of a call to string.Create(...) since that's intended for allocation-free string creation. On the one hand, it does read as much simpler but that makes the potential allocation even more subtle. Personally, I probably wouldn't have cut this particular corner, but I do understand your thinking.
| tabs.Span.CopyTo(destination); | ||
| spaces.Span.CopyTo(destination[tabCount..]); |
There was a problem hiding this comment.
Did you consider using Span<T>.Fill(...)? It feels like the body of this lambda could be replaced with:
destination[..tabCount].Fill('\t');
destination[tabCount..].Fill(' ');That'd perform vectorized writes without allocating a new string.
There was a problem hiding this comment.
FYI, that's essentially what new string('\t', tabCount) does to fill the string contents.
There was a problem hiding this comment.
Oooh, that's beautiful!
3 participants
There are typically only a few different sizes requested, and useTabs/tabSize very rarely change. Locally, I only saw eight unique combinations of these values when opening and typing in orchardcore.
ComputeIndent accounts for 11.0 MB (0.4%) of total allocations and 27 ms in in the ScrollingAndTypingInCohosting speedometer test. With this change, the cost under IndentCache.GetIndentString goes to 0.0 MB (0.0%) and 13 ms respectively.
Test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/680751