Pool CodeWriter ReadOnlyMemory<char> pages #10585
Conversation
These allocations are present in a customer trace I'm looking at, accounting for 1.4% of allocations in the VS process (around 100 MB). The owner of the CodeWriter is already disposable, so making the CodeWriter disposable is trivial, and allows for all ReadOnlyMemory<char> pages added to _pages to be released back to a pool.
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriter.cs
Show resolved
Hide resolved
...osoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/DefaultCodeRenderingContext.cs
Outdated
Show resolved
Hide resolved
Code review feedback: Move CodeWriter ctor to make code cleaner
|
@dotnet/razor-compiler -- ptal |
There was a problem hiding this comment.
Did you go through all CodeWriter type references to make sure we are disposing CodeWriter instances now? It looks like RazorHtmlWriter is owning a CodeWriter but not disposing it.
There was a problem hiding this comment.
Just went through and in addition to the RazorHtmlWriter usage, there were a bunch of test file usages that I changed too. Thanks!
|
|
||
| if (_pageOffset == 0) | ||
| { | ||
| lastPage = ArrayPool<ReadOnlyMemory<char>>.Shared.Rent(PageSize); |
There was a problem hiding this comment.
Consider renaming PageSize to MinimumPageSize to signal that the page length might not be be the same as the requested size.
|
|
||
| // Add our chunk of text (the ReadOnlyMemory<char>) and increment the offset. | ||
| lastPage[_pageOffset] = value; | ||
| _pageOffset++; | ||
|
|
||
| // We've reached the end of a page, so we reset the offset to 0. | ||
| // This will cause a new page to be added next time. | ||
| if (_pageOffset == PageSize) | ||
| if (_pageOffset == lastPage.Length) |
There was a problem hiding this comment.
Good change, though it's a bit subtle. Could you add a comment or two describing this behavior?
There was a problem hiding this comment.
Comment added, hopefully more obvious now.
2) Rename PageSize => MinimumPageSize 3) Add comment around subtle comparison
|
@jjonescz -- any remaining concerns / requests? |
These allocations are present in a customer trace I'm looking at, accounting for 1.4% of allocations in the VS process (around 100 MB).
The owner of the CodeWriter is already disposable, so making the CodeWriter disposable is trivial, and allows for all ReadOnlyMemory pages added to _pages to be released back to a pool.
*** Customer trace ***
*** local trace WITHOUT changes opening OrchardCore ***
*** local trace WITH changes opening OrchardCore ***
