-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Simplify and optimize some GraphNode find functions #5850
Simplify and optimize some GraphNode find functions #5850
Conversation
Do you have a benchmarks with various hierarchy cases? |
I ran somes on one of my projects which has a hierarchy of ~800 nodes.
So it may be sometimes a bit slower when only a few nodes need to be checked, but it gets way faster on bigger hierarchies. |
This PR seems to me a wonderful example of how simplifying code can make it more readable, maintainable and run faster all at the same time. Great job @NewboO !! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I'm happy to merge this. @slimbuck ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks. 🙏
Co-authored-by: Will Eastcott <will@playcanvas.com>
Follow-up of #4603
This PR simplifies
GraphNode.find
andGraphNode.findOne
.It also avoids creating and merging lots of arrays in
GraphNode.find
which means less garbage.In addition,
GraphNode.findByName
now simply callsGraphNode.findOne
so the traversing logic isn't duplicated anymore.I confirm I have read the contributing guidelines and signed the Contributor License Agreement.