HBASE-29103 Avoid excessive allocations during reverse scanning when seeking to next row #6643
+21
−17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…seeking to next row
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
^ I believe the above test failure is due to a flakey test. When running locally, the test is flakey for me. I can investigate more |
|
I've confirmed that the above test failure is because That's to say: all test failures are unrelated. |
|
@Apache9 tagged you here because I don't know much about this code. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
This PR updates the reverse scanning flow to call HFileScanner#getKey instead of HFileScanner#getCell for Cells that later have PrivateCellUtil.createFirstOnRow() called on them. The aim here is that we'll reduce excessive memory usage/allocations as:
previousRowis in use, it will be a KeyOnlyKeyValue rather than a full ExtendedCellThis should be safe as we always call HFileScanner#getKey in a context where the HFileScanner is seeked so it won't throw.
Implementation Notes
While I was making updates here, I noticed some invalid javadoc in the reverse scanning code path which I fixed up as a part of this issue. I can separate those changes out into a different issue if that's preferred.
Testing
I ran a lot of the reverse scan specific related tests locally and they passed, however, I would like to run the entire test suite against this patch to ensure that it's correct. Since this affects the meta read hotpath, we want to ensure that no edge cases are missed.
Performance Impact
On a region server that was under extreme CPU load (100%), an allocation profile (see linked JIRA) revealed that about 3% of allocations for a reverse scan heavy workload over a table that was using FAST_DIFF data block encoding came from the BufferedDataBlockEncoder#getCell method. This table had moderate sized rows: about ~1kb on average with 5-6 columns and a block size of 128kb.
I would expect this performance optimization to really only materially aid performance on tables with reverse scan workloads that have column values that aren't trivial in size (more than a couple of bytes). I need to do some more in-depth profiling though to come up with concrete numbers here.
Benchmarks
I've benchmarked this patch vs. the master branch and posted the results in the linked JIRA.
HBASE-29103