-
Notifications
You must be signed in to change notification settings - Fork 3.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
HBASE-27225 Add BucketAllocator bucket size statistic logging #4637
Merged
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
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.
Mind teach me briefly on this math here? Aren't
bucketCapacity
andbucketObjectSize
computed for each individual bucket? If so, why are we multiplying by the number of buckets that already have some data? Wouldn't this (bucketCapacity % bucketObjectSize) differ for each bucket?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.
Happy to, and feel free to tell me if I'm misunderstanding here. I'm also learning the BucketCache recently.
bucketCapacity
is calculated once for the whole cache. It's defined as4 * largestBucketSize
.bucketCapacity
well, leaving a remainder.freeBucket
(which is probably better defined as afillingBucket
) and a number offullBuckets
. A fullBucket isn't actually full, depending on the block size it's allocated to. The remainder for the block size will be empty/unused.Let's use my example snippet from above:
6295552 % 525312 = 517120
. So given that we always allocate blocks with size 525312, we basically are missing out on ~1 block per bucket allocated for this size. There are only approx6295552 / 525312 = 11
blocks fitting into this bucket size, so that almost 10% waist.6295552 % 66560 = 38912
.Buckets can be reallocated over time. Once they become a
completelyFreeBucket
, the next allocation that needs one, of any size, will take that bucket and reconfigure it for its purpose. At that point the amount of waist for that bucket would change based on the new block size.[*] getting back to "even if the actual block size isn't exactly that amount". This speaks to another source of waist which is harder to calculate. So the waistBytes is actually very much an underestimation, but I'm not sure by how much. Let's say you have a block with 150k size. That doesn't fit into the 129k bucket, so it has to go into the 513k bucket. But even though the block size is 150k, we need to allocate a full 513k, leaving 363k of waisted space.
I say this is harder to calculate, but of course it'd be relatively easy. When we call
roundUpToBucketSizeInfo
we could just subtract the blockSize from the bucketSize, and then add that diff to a histogram. But I was just thinking this might be a very hot codepath and adding a histogram there will be a lot more expensive than what I currently have since currently the statistics are calculated totally off the hot path.I imagine maybe the unified.encoded.blocksize could help with this problem, but also part of the reason for so much upward skew for us is because we have some users writing large rows. So may not entirely help us at least.
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.
Ok, so this waste here is a best case scenario, not taking into account the real size of allocated blocks on each bucket? Considering that real world cases may not have perfectly sized 512KB blocks, fragmentation would be even higher?
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.
yea, correct. fragmentation could make this worse, and its harder to cheaply calculate
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.
if you have ideas, i'd be happy to try adding that. One thought I had was to add a LongAdder to each BucketSizeInfo, and increment it in
allocateBlock(int blockSize)
. But really we should also be decrementing somewhere. Possibly infreeBlock
, but we'd need to add the blockSize as an argument there. And there's a question of the impact on performance (probably small/worth it?)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 just implemented the above in my latest patch. I'm thinking a LongAdder increment/decrement is probably not going to make a noticeable difference next to everything else.
Here's an example output from the test, unfortunately I don't have a real-world example:
We could combine wasted + fragmented. Probably confusing to have both, but also useful once you understand the difference (which we can document)