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

Hide the internal data structure of HeapPointWriter #12762

Merged
merged 6 commits into from Nov 24, 2023

Conversation

iverase
Copy link
Contributor

@iverase iverase commented Nov 5, 2023

HeapPointWriter uses a a byte array to hold points on heap. This array is access directly from BKD radix selector to sort points in place. In addition the HeapPointReader uses the array to read the points too.

I wanted to check the performance impact of changing this data structure as this array can be very big which plays bad with garbage collector like G1 as the array will be considered humongous. The current structure of HeapPointWriter makes this very difficult.

This PR refactor HeapPointWriter by adding methods for in place sorting and changes HeapPointReader to reads points using a functional interface. It encapsulates the internal byte array inside HeapPointWriter. The nice side effect is it removes some code duplication too.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Are you seeing performance implications from this refactoring? If not this looks good to me, let's just add some javadocs to the new APIs on HeapPointWriter?

@iverase
Copy link
Contributor Author

iverase commented Nov 24, 2023

I didn't notice any performance change. Added javadocs.

@iverase iverase merged commit 74085cd into apache:main Nov 24, 2023
4 checks passed
@iverase iverase deleted the HeapPointWriter branch November 24, 2023 12:58
@iverase iverase added this to the 9.9.0 milestone Nov 24, 2023
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.

None yet

2 participants