-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29252: Reduce allocations in RowIndexSeekerV1 #6902
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
HBASE-29252: Reduce allocations in RowIndexSeekerV1 #6902
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
hbase-common/src/main/java/org/apache/hadoop/hbase/io/encoding/RowIndexSeekerV1.java
Show resolved
Hide resolved
05789be to
22e8d01
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
test failures are unrelated |
| protected int nextKvOffset; | ||
| // buffer backed keyonlyKV | ||
| // buffer backed keyonlyKV, reset and re-used as necessary to avoid allocations | ||
| private ByteBufferKeyOnlyKeyValue currentKey = new ByteBufferKeyOnlyKeyValue(); |
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.
Make it final if we do not need to recreate it?
|
|
||
| protected ByteBuff currentBuffer; | ||
| protected int startOffset = -1; | ||
| protected int keyOffset = -1; |
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.
Better add some comments to explain what do these offsets and lengths mean?
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.
I won't request further changes, but for future reference, these comments would ideally be javadoc and then this additional information would be more readily onhand for folks pursuing the code in an IDE environment.
| protected int valueLength; | ||
| // Tags start after values and end after tagsLength | ||
| protected int tagsLength = 0; | ||
| protected int tagsOffset = -1; |
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.
I noticed that tagsOffset was effectively unused so I've removed it
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
|
||
| protected ByteBuff currentBuffer; | ||
| protected int startOffset = -1; | ||
| protected int keyOffset = -1; |
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.
I won't request further changes, but for future reference, these comments would ideally be javadoc and then this additional information would be more readily onhand for folks pursuing the code in an IDE environment.
Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
Signed-off-by: Nick Dimiduk <[email protected]>
I've looked at a lot of allocation profiles of RegionServers doing a read-heavy workload. Some allocations that dominate the chart can be easily avoided.
The following code in the main decode method
results in a new ByteBuffer for every cell. The reason to have this duplicate ByteBuffer is to hold the result of tmpPair.getSecond() as its position state. But this is just an integer that can be more cheaply stored in a different way. We can introduce a current.keyOffset variable and do this instead:
and then reference current.keyOffset where we previously referenced current.keyBuffer.position().
Additionally,
RowIndexSeekerV1.SeekerStatecontains aByteBufferKeyOnlyKeyValuefield that is replaced on every cell read. This object can be reset and re-used instead.