Skip to content

perf(TreeView): add memoization + test demo#6362

Merged
tlabaj merged 5 commits into
patternfly:mainfrom
kmcfaul:treeview-memo
Oct 14, 2021
Merged

perf(TreeView): add memoization + test demo#6362
tlabaj merged 5 commits into
patternfly:mainfrom
kmcfaul:treeview-memo

Conversation

@kmcfaul
Copy link
Copy Markdown
Contributor

@kmcfaul kmcfaul commented Sep 27, 2021

What: Possible improvement for #6268

In this PR:

  • Adds memoization to TreeViewListItem
  • useMemo and the new Tree view demo page under the Demos section are for demonstrating this memoization only and should not be merged. I will remove them if we want to proceed with the memo changes.

TreeView will completely re-render all nodes when the onSelect is called and a state update is made to activeItems, because a state update is being made at the top level of the recursive tree view. To help performance, I've added memoization to the TreeViewListItem to alleviate some of the unnecessary re-rendering - items should only re-render when their active state is going to change.

The main caveat in adding this memoization logic is due to TreeView's recursive nature, parent nodes that do not see an update to their rendering will not re-render (or even evaluate) child nodes below them, and re-rendering when the children prop updates defeats the purpose as the whole tree would re-render. The current working solution has activeItems change slightly - instead of passing only the selected item, all nodes on the path from the top node to the selected node must be passed to this property. This doesn't change activeItems effective behavior (only the leaf node's css will change as usual) but it would be a breaking change to the current demos (unless the useMemo prop is kept and has a default value of false, which would disable memoization unless it is required).

Any other thoughts and ideas regarding tree view performance is welcome!

@kmcfaul
Copy link
Copy Markdown
Contributor Author

kmcfaul commented Sep 27, 2021

An unrelated but possible alternative for large tree view datasets would be to mock a tree-like view using a simple list structure instead of using recursion - that would also separate the state out from the stack. This would require a way to provide indentation to a nested ul based on depth, which I'm not sure about but may be possible?

@patternfly-build
Copy link
Copy Markdown
Collaborator

patternfly-build commented Sep 27, 2021

PF4 preview: https://patternfly-react-pr-6362.surge.sh

@kmcfaul
Copy link
Copy Markdown
Contributor Author

kmcfaul commented Sep 27, 2021

@kmcfaul kmcfaul marked this pull request as draft September 27, 2021 17:35
@nicolethoen
Copy link
Copy Markdown
Contributor

I can definitely notice a difference. I'm not sure how much more noticeable it'll be with more complicated treeListItems. And I don't know if the improvement is as good as the customers will want.

As far as memoization goes, you're saying that you need to evaluate if activeItems has changed to know if the children should rerender, right? And mayyyyybe we could further improve this by finding a less expensive way to make that determination?

@kmcfaul
Copy link
Copy Markdown
Contributor Author

kmcfaul commented Sep 29, 2021

Yeah that's why I think it may be worth also investigating if we can make a faux-tree view with a simple list over the recursion.

For the memoization yes. activeItems is really the only prop that will warrant a rerender since the folder expansion is internal so that's what I used for the check logic.

@kmcfaul kmcfaul marked this pull request as ready for review October 6, 2021 15:53
@kmcfaul
Copy link
Copy Markdown
Contributor Author

kmcfaul commented Oct 8, 2021

Note - test few folders, shallow tree with lots of child nodes per folder (something like 3 folders, 10k+ nodes per folder). Large shallow trees may be a good starting point for trying out a faux-tree "list" component as well.

Copy link
Copy Markdown
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read through the React.memo docs and think the comparator function could be better

Comment thread packages/react-core/src/components/TreeView/TreeViewListItem.tsx Outdated
return false;
}

return !(prevIncludes || nextIncludes);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you also want to also shallowly compare the rest of prepProps and nextProps?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and tested. compareItems and children always change on any state update so I'm excluding those at the moment - because children is another TreeView and should use its own memoization and compareItems is set by default and registering as a new function every comparison despite being the same code. Not sure if there's a better way of comparing compareItems.

Copy link
Copy Markdown
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @kmcfaul

@tlabaj tlabaj merged commit 6e35627 into patternfly:main Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants