Reuse text layout for performance #2980
Conversation
There was a problem hiding this comment.
This seems reasonable to me, but I'd definitely want @miniksa to sign off on it.
|
I'm thinking there should be some time that |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
You will probably need to merge in |
| _runs.clear(); | ||
| _runIndex = 0; | ||
| _text = L""; | ||
|
|
There was a problem hiding this comment.
Should probably also reset _breakpoints and _numberSubstitution just for completeness.
| @@ -26,6 +26,8 @@ namespace Microsoft::Console::Render | |||
| const std::basic_string_view<::Microsoft::Console::Render::Cluster> clusters, | |||
| size_t const width); | |||
|
|
|||
| void SetData(std::basic_string_view<Cluster> const clustor); | |||
There was a problem hiding this comment.
| void SetData(std::basic_string_view<Cluster> const clustor); | |
| void SetData(std::basic_string_view<Cluster> const clusters); |
typo and didn't match cpp name
| _glyphCell.cx); | ||
| if (_drawingTextLayout == nullptr) | ||
| { | ||
| _drawingTextLayout = ::Microsoft::WRL::Details::Make<CustomTextLayout>(_dwriteFactory.Get(), |
There was a problem hiding this comment.
Why is this Microsoft::WRL::Details::Make instead of Microsoft::WRL::Make?
| { | ||
| _drawingTextLayout = ::Microsoft::WRL::Details::Make<CustomTextLayout>(_dwriteFactory.Get(), | ||
| _dwriteTextAnalyzer.Get(), | ||
| _dwriteTextFormat.Get(), |
There was a problem hiding this comment.
Should we be using Microsoft::WRL::Make for all the other ComPtrs we make in this class too? If so, maybe we should file a follow up work item.
| _dwriteFontFace.Get(), | ||
| clusters, | ||
| _glyphCell.cx); | ||
| if (_drawingTextLayout == nullptr) |
There was a problem hiding this comment.
Nothing is invalidating the _drawingTextLayout when the other parameters change between frames.
For instance, if _dwriteFontFace got updated between frames for a different font, we wouldn't recreate the layout. We would need to make sure that this is invalidated for any of the parameters that are now "cached" between frames inside the _drawingTextLayout.
There was a problem hiding this comment.
There was a problem hiding this comment.
And let’s also take a look at #2960 which is even more harmless but still powerful
|
After some investigation, the performance gain with this doesn't seem to be work the effort. Even though this PR eliminate the object creating and destroying, the |
Summary of the Pull Request
Another possible performance issue here.
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
This is a draft. Just showing some idea.
Validation Steps Performed