A more efficient copy assignment operator for Pane.LayoutSizeNode #15169
Conversation
…youtSizeNode. Eliminate now unneeded _AssignChildNode code block
microsoft-github-policy-service
bot
added
Issue-Task
| firstChild = other.firstChild ? std::make_unique<LayoutSizeNode>(*other.firstChild) : nullptr; | ||
| secondChild = other.secondChild ? std::make_unique<LayoutSizeNode>(*other.secondChild) : nullptr; | ||
| nextFirstChild = other.nextFirstChild ? std::make_unique<LayoutSizeNode>(*other.nextFirstChild) : nullptr; | ||
| nextSecondChild = other.nextSecondChild ? std::make_unique<LayoutSizeNode>(*other.nextSecondChild) : nullptr; |
There was a problem hiding this comment.
This is definitely a lot shorter, but it's not more efficient by any means. It's actually significantly less efficient. Although I bet it doesn't really matter, since panes aren't really that widely used in the first place.
There was a problem hiding this comment.
Huh.. Looks like you are right according to compiler explorer the same code is generated for both versions. Thats interesting so thanks for pointing it out. Still can eliminate an entire block of code :)
There was a problem hiding this comment.
Nope. Not the same code (I did something wrong in compiler explorer). Interesting!!
There was a problem hiding this comment.
Alright wait - is it more efficient or is it not? Leonard, you signed off on the central thesis of this PR but left a comment indicating that it didn't do anything of the sort?
There was a problem hiding this comment.
The new code is less performant because it recreates any heap allocations even if they already existed. The old code didn't do that. But it is more efficient in a way, because it's less code. If we want to optimize for that, then this PR is also a win. But if it's about performance then we should keep the old code and only fix the .release() call.
Using std::unique_ptrs to build a binary tree is not optimal in the first place however, so any sort of performance optimizations on this is a bit of a lost cause anyways. 😅
The correct way to store small binary heaps like that is to store them as a flat list. That is, at index 0 is the root, index 1 and 2 store its left and right splits, etc. There's no need for references or pointers or anything to traverse such a binary tree because the position of the 2 child nodes can be determined based on the position of the parent node. This style of binary tree representation is actually more common than the classic text book pointer-style tree we're using. It's described here: https://en.wikipedia.org/wiki/Binary_tree#Arrays
zadjii-msft
added
the
AutoMerge
4 participants
Summary of the Pull Request
This pull request updates the implementation of the copy assignment operator for Pane::LayoutSizeNode to a more efficient version and eliminates the need for the _AssignChildNode code block.
References and Relevant Issues
#11965 #11963
Detailed Description of the Pull Request / Additional comments
My understanding of the discussion and intent of the two linked issues is that this is a more efficient way to implement the copy assignment operator for Pane.LayoutSizeNode and eliminates the need for the code block _AssignChildNode. Since both were relatively small changes, I combined the two in one PR. If that is not desirable, I can separate them. All existing tests continue to pass.
Validation Steps Performed
All existing tests pass. No visible changes in behavior of the terminal.
PR Checklist