Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TreeView] Improve performance by removing getNavigableChildrenIds method #12713

Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Apr 9, 2024

Requires to review #12729 before because it is merged inside this one.

Extracted from #12703
Part of #10300

The instance.getNavigableChildrenIds was looping on all the items to check if they were navigable or not and it was used a lot.
For exemple, sometime several times per item when generating a range of items.

@flaviendelangle flaviendelangle added performance component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! labels Apr 9, 2024
@flaviendelangle flaviendelangle self-assigned this Apr 9, 2024
: [itemBId, itemAId];
};

export const getNavigableItemsInRange = (
Copy link
Member Author

Choose a reason for hiding this comment

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

I added an index lookup in #12213, I will extract it into a standalone PR

* Another way to put it is which item is shallower in a trémaux tree
* https://en.wikipedia.org/wiki/Tr%C3%A9maux_tree
*/
const findOrderInTremauxTree = (
Copy link
Member Author

Choose a reason for hiding this comment

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

Pure copy / paste

@mui-bot
Copy link

mui-bot commented Apr 9, 2024

Deploy preview: https://deploy-preview-12713--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 19536f3

@flaviendelangle flaviendelangle marked this pull request as ready for review April 9, 2024 07:33
: [itemBId, itemAId];
};

export const getNavigableItemsInRange = (
Copy link
Member Author

Choose a reason for hiding this comment

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

I added an index lookup in #12213, I will extract it into a standalone PR

@LukasTy LukasTy changed the title [TreeView] Improve performances by removing getNavigableChildrenIds method [TreeView] Improve performance by removing getNavigableChildrenIds method Apr 10, 2024
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM. 👏
I didn't do a super thorough testing, but code and behavior seems to check out. 👌

@flaviendelangle
Copy link
Member Author

This part of the Tree View is probably the most tested, which makes me quite confident, but yeah it's a non trivial changer 😬

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 10, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 10, 2024
@flaviendelangle flaviendelangle merged commit 73dadbd into mui:master Apr 11, 2024
17 checks passed
@flaviendelangle flaviendelangle deleted the remove-getNavigableChildrenIds branch April 11, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants