Repository navigation
perf(tree): keep tree-selection linear in the number of rows (#DS-5796) - #2255
Open
nazirgitwork wants to merge 2 commits into
Open
nazirgitwork wants to merge 2 commits into
nazirgitwork wants to merge 2 commits into
Conversation
…(#DS-5796) Re-rendering the rows searched the selected values once per row, costing rows × selected. The values now go into a Set. The tree also completes optionFocusChanges and optionBlurChanges on destroy, so their subscribers complete with it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
KbqTreeSelectiondid work proportional to the number of rows several times on each render, and some of it for each row. In a large tree that work grew with the square of the number of rows. This PR removes the quadratic paths and the per-row subscriptions. It also stopsmouseenterfrom running change detection in trees outside a select.List of notable changes:
KbqTreeSelection: options are indexed once into a by-node map and a by-value map. Before,unorderedOptionswas searched for every node, which was O(n²). As before, the first option wins in each map, and a node match takes precedence over a value match.optionFocusChanges/optionBlurChanges: each one is now a single stream that options report to through the newKbqTreeOptionParent.reportOptionFocus()/reportOptionBlur()methods. Before, every call merged the streams of all rendered options, and the tree re-subscribed after each render. That cost O(n) per render, and a subscriber never received focus events from rows rendered after it subscribed. It now does.delay(0), so a row focused in that gap was missed.KbqTreeOption:mouseenteris no longer a host listener. It is bound outside the Angular zone and re-enters the zone only when the tree is inside a select, the only case that reacts to it. A tree outside a select no longer checks views for every row the pointer crosses.mouseenteroutside a select, focus of a newly rendered row,optionFocusChangesreporting rows rendered after subscribing, and keyboard order across expand, collapse and new data when rows share values.check-api-docsbaseline: two new members, andoptionFocusChanges/optionBlurChangesare now documented.What should reviewers focus on?
KbqTreeOptionParentgains two required methods. Code that provides its own implementation ofKBQ_TREE_OPTION_PARENT_COMPONENTwill stop compiling until it adds them. Is that acceptable here, or should they be optional?optionFocusChanges/optionBlurChangesnow also emit for rows rendered after the subscription. Before, the observable covered only the rows rendered when the getter was called.mouseenterlistener runs outside the zone. Please check it in zoneless apps and inkbq-tree-select, where hovering a row still has to move focus.