Cache VT buffer line string to avoid (de)alloc on every paint #6840
Conversation
miniksa
added
Product-Conpty
|
Before: After: |
miniksa
changed the title
| @@ -479,8 +479,7 @@ using namespace Microsoft::Console::Types; | |||
| RETURN_IF_FAILED(_MoveCursor(coord)); | |||
|
|
|||
| // Write the actual text string | |||
| std::wstring wstr = std::wstring(unclusteredString.data(), cchActual); | |||
| RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8(wstr)); | |||
| RETURN_IF_FAILED(VtEngine::_WriteTerminalUtf8({ _bufferLine.data(), cchActual })); | |||
There was a problem hiding this comment.
is it not possible to pass the wstring directly as a view? bah.
were we literally making a whole copy of the wstring to a wstring?
There was a problem hiding this comment.
I don't know why the copy was there. It's probably because the _WriteTerminalUtf8 function converted to using std::wstring_view from std::wstring at a point in time after this paint function was last updated and this became a mechanical replace.
As for why we don't pass _bufferLine directly, the cchActual may be less than the length of the full buffer depending on trimming calculations performed above.
| @@ -204,6 +204,9 @@ namespace Microsoft::Console::Render | |||
|
|
|||
| bool _WillWriteSingleChar() const; | |||
|
|
|||
| // buffer space for these two functions to build their lines | |||
| // so they don't have to alloc/free in a tight loop | |||
| std::wstring _bufferLine; | |||
There was a problem hiding this comment.
We discussed this a bit in Teams. This should max out at approximately (buffer width * sizeof(wchar_t) * 2). We could try to shrink it back down when we start getting small regions, but it would probably grow again when more complicated painting follows up immediately after. The amount of memory left around is arguably small for how much benefit we get from not doing the alloc/dealloc.
I could come up with some sort of time-based decay of the size of the allocation (if someone was doing big draws and has now been doing little draws for a while, it could release it back.... but I don't really have a justification to do all that to pay back ~240 bytes of memory.)
Also, we only have til::manage_vector not til::manage_string at this time. So we'd have to write something for that.
We decided together that this is a big MEH.
DHowett
changed the title
|
🎉 Handy links: |
PR Checklist
Detailed Description of the Pull Request / Additional comments
A lot of time was spent between each individual line in the VT paint engine in allocating some scratch space to assemble the clusters then deallocating it only to have the next line do that again. Now we just hold onto that memory space since it should be approximately the size of a single line wide and will be used over and over and over as painting continues.
Validation Steps Performed
time cat big.txtunder WPR. Checked before and after perf metrics.