HBASE-24637 - Reseek regression related to filter SKIP hinting #2663
+96
−6
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
ramkrish86
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
apurtell
requested changes
Nov 16, 2020
| @@ -786,6 +791,19 @@ private void updateMetricsStore(boolean memstoreRead) { | |||
| } | |||
| } | |||
|
|
|||
| private void doSeekCol(Cell cell) throws IOException { | |||
| // we check when ever a seek_next_col happens did the seek really land in a new block. | |||
There was a problem hiding this comment.
I'm not sure this comment helps with understanding. (It doesn't for me, but could just be me...)
Rewrite it?
Questions to be answered by the comment:
- What does this optimization do? Why next() instead of seekOrSkip...?
- Is this the right layer to be doing this?
- Why does this optimization have to be done here instead of down in the file scanner?
There was a problem hiding this comment.
What does this optimization do? Why next() instead of seekOrSkip...?
Next is always good if we know we are going with the actual next block only. seekOrSkip involves loading the block and also doing a seek to the given key. Not only that the code to do that decision does lot of compares. That adds to the performance. Here we try to do a decision making with the fact that if the seekOrSkip has really landed in the actual next block only- we tend to take that as the pattern for the rest of the scan and go ahead with next and avoid all those loading of the block and seeking of the block.
Why does this optimization have to be done here instead of down in the file scanner?
The reason is that I felt that the trackers are deciding what the store scanner should be doing and hence asking to do a SEEK or NEXT. The storescanner layer is trying to do the actual smart work of deciding whether to really do a seek or next as per what the trackers are saying. Hence I added that logic at this layer.
| seekAsDirection(matcher.getKeyForNextColumn(cell)); | ||
| if (prevIndexKeyNull) { | ||
| // even if one seek has lead to another block - reset to false. |
There was a problem hiding this comment.
This comment should be rewritten or removed. Ideally not removed, because you are trying to communicate something important for understanding how the code works. Try a rewrite?
| @@ -205,6 +207,9 @@ private StoreScanner(HStore store, Scan scan, ScanInfo scanInfo, | |||
| this.scanUsePread = this.readType != Scan.ReadType.STREAM; | |||
| } | |||
| this.preadMaxBytes = scanInfo.getPreadMaxBytes(); | |||
| // TODO : Introduce config here at the ScanInfo level. Determine based on number of blocks | |||
There was a problem hiding this comment.
This would be something deeply internal that no user could meaningfully set unless they read all the code and become a wizard. Please figure this out based on current configuration.
No new config!
|
Also I assume there is some kind of performance analysis that indicates an improvement? If not please provide one. |
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks @apurtell - I will update the patch with test results and perf numbers. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
|
@infraio - Ping for review. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
infraio
reviewed
Dec 9, 2020
| // which is updated in the method 'seekOrSkipToNextColumn'. Do this when rowColBloom | ||
| // is not used | ||
| if (this.matcher.isUserScan() && !get && !useRowColBloom && seekToSameBlock | ||
| && bytesRead > switchToNextOnlyBytes) { |
There was a problem hiding this comment.
Why need "bytesRead > switchToNextOnlyBytes"? Can do it directly?
There was a problem hiding this comment.
@infraio - Thanks for the review. Can you tell me more when you say 'do it directly'? Without observing for few blocks how to know whether the addColumns is actually projecting all/most of the columns or say it is projecting in such a way that instead of the nextblock we land in next+1 block()? So this is more like an observation we do and then decide if to take an action based on that observation.
There was a problem hiding this comment.
this.switchToNextOnlyBytes = this.preadMaxBytes. Then why use preadMaxBytes to decide this?
There was a problem hiding this comment.
Oh I got your point. The reason to do that was to see if we can introduce a new config for that instead of using preadMaxBytes config. I have not added a config (ie. a TODO). That config can be for now directly referring to preadMaxBytes or may be make it a new value.
|
@infraio |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Will take a look this week. Can go ahead if you have other approvers and I haven't replied in time. Thanks. |
infraio
reviewed
Dec 15, 2020
| * with seek(). Currently it defaults to the STORESCANNER_PREAD_MAX_BYTES config | ||
| * To disable this feature put a value < 0. | ||
| */ | ||
| public static final String HBASE_SWITCH_TO_NEXT_AFTER_BYTES_READ = |
There was a problem hiding this comment.
The default HBASE_SWITCH_TO_NEXT_AFTER_BYTES_READ should smaller than one block size? But STORESCANNER_PREAD_MAX_BYTES is 4 times of block size.
There was a problem hiding this comment.
Currently am having 4 * blockSize as the default value now. I cannot have it less than one block size because unless I traverse multiple blocks I cannot make out this pattern. Within one block we cannot decide. I may be not getting your Q clearly - if that is the case do let me know I can try to explain it.
There was a problem hiding this comment.
I check your example in PPT. What this PR want to reduce is the compare times of "matcher.compareKeyForNextRow(nextIndexedKey, cell)", right?
There was a problem hiding this comment.
The number of compares we do is the real reason for the perf regression. So we are trying to reduce that. When I say perf regression it is specifically for the case where the filter says SKIP but the SQM says NEXT_COL. This will in general improve performance of a case where we have INLCUDE_NEXT_COL when we project all columns in a scan query. (that perf should be same in 1.x also).
infraio
reviewed
Dec 15, 2020
| @@ -728,7 +739,7 @@ public boolean next(List<Cell> outResult, ScannerContext scannerContext) throws | |||
| break; | |||
|
|
|||
| case SEEK_NEXT_COL: | |||
| seekOrSkipToNextColumn(cell); | |||
| doSeekCol(cell); | |||
There was a problem hiding this comment.
I thought it is not need to change this method name? You can add the new logic to seekOrSkipToNextColumn method directly?
There was a problem hiding this comment.
Ok . I can see how can that be done.
|
@ramkrish86 Any update here sir? |
|
Any update here? |
No new config. Repeating my earlier comment. What user is actually going to understand and set this? Figure it out without user intervention. Please don't merge until this is addressed. |
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.
This patch depends on what the scans or reads does with the blocks till the configured size. Just for understanding or initial review I have based this on the pread size (which is 4 * block size). This needs a better measure may be based on the number of blocks purely rather than the block size. But the idea is that if based on that config we find that the seekOrSkipToNextcol() has always landed in the immediate next block always go with next() rather than this seekToSkiptoNextcol(). In our tests it is revealed that the comparison that we do here in case of ensuring whether to seek or skipping is adding to the performance rather than the actual seek.