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

Improve performance of ShortNameComparator. #1027

Merged

Conversation

jdcove2
Copy link
Collaborator

@jdcove2 jdcove2 commented Dec 19, 2024

This is an updated version of #135 .

@ldhardy ldhardy self-requested a review December 20, 2024 10:28
@jpdahlke jpdahlke added this to the v8.19.0 milestone Dec 20, 2024
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Do we think the performance improvements will be primarily speed, memory utilization, or both?

Copy link
Collaborator

@ldhardy ldhardy left a comment

Choose a reason for hiding this comment

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

Is there a trick to recreate the profile you describe in #135 ? I've been trying to use the profiler on our main branch to observe the described behavior and I'm not having any luck. I guess there have to be a lot of children extracted - I tried dumping in all kinds of archive input to see if I can see something similar on the profiler, but I'm not observing what you were.

@jdcove2
Copy link
Collaborator Author

jdcove2 commented Dec 23, 2024

Yes, this optimization is designed for objects with a large number of children (i.e. 100,000 or more). Let me know if you would like me to generate an object for you.

@ldhardy
Copy link
Collaborator

ldhardy commented Dec 23, 2024

Yes, this optimization is designed for objects with a large number of children (i.e. 100,000 or more). Let me know if you would like me to generate an object for you.

Nah, I believe you. It's not that big of a change really.

@jdcove2
Copy link
Collaborator Author

jdcove2 commented Dec 23, 2024

Based on the profiling results in the original PR, this change should improve both speed and memory usage as in the normal case of sorting numerical children it creates virtually no objects.

@ldhardy
Copy link
Collaborator

ldhardy commented Dec 23, 2024

Yes, this optimization is designed for objects with a large number of children (i.e. 100,000 or more). Let me know if you would like me to generate an object for you.

I meant the algorithm is basically the same, but yes, I can see how it's a less heavy handed implementation.

@jpdahlke jpdahlke added the enhancement An enhancement or update to an existing feature label Dec 23, 2024
@jpdahlke jpdahlke merged commit e69fde9 into NationalSecurityAgency:main Dec 23, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or update to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants