HBASE-24850 CellComparator perf improvement #2747
Conversation
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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
I see you are just generalizing the BBKVComparator trick so it works for KeyValue as well as the BBKV. I like the speedup. I don't like the proliferation of branches: if BBKV, if unsafe, if onheap, if tags, if seqid, if extended cell.
We should fix this. A Cell implementation that does lazy length caching internally? Then we'd not need comparator knowing about implementation? What you think Ram?
| @@ -161,11 +161,11 @@ private int getTimestampOffset() { | |||
|
|
|||
| @Override | |||
| public byte getTypeByte() { | |||
| return ByteBufferUtils.toByte(this.buf, this.offset + this.length - 1); | |||
| return getTypeByte(this.length); | |||
There was a problem hiding this comment.
this.length is updated as we parse? Presumes we read Cell parts in order... first the row and then the CF and then the qualifier? We are not allowed to just and read the qualifier w/o first reading row?
There was a problem hiding this comment.
But maybe this.length doesn't change? Its the key length?
There was a problem hiding this comment.
In this case it is the keylength only because it is BbKeyOnlyKV.
There was a problem hiding this comment.
Ok. Maybe if a new patch, rename this data member to keyLength?
| } | ||
|
|
||
| @Override | ||
| public void setSequenceId(long seqId) throws IOException { | ||
| public void setSequenceId(long seqId) { | ||
| throw new IllegalArgumentException("This is a key only Cell"); |
| @@ -292,4 +287,49 @@ public long heapSize() { | |||
| } | |||
| return ClassSize.align(FIXED_OVERHEAD); | |||
| } | |||
|
|
|||
| @Override | |||
| public int getFamilyLengthPosition(int rowLen) { | |||
There was a problem hiding this comment.
This stuff used to be private. Making it public exposes the implementation. You fellows did a mountain of work making it so we could CHANGE the implementation. This goes back on that work? At least exposing this stuff as public methods?
There was a problem hiding this comment.
For instance, what if a Cell implementation had data members that cached all lengths... a column family length data member and a row length data member, etc. These methods wouldn't make sense to it?
There was a problem hiding this comment.
What if this stuff just stayed internal? I think you said, I did the KV one and here you are doing BB. Do we have to do more? There'd be duplication... or we could call out to a utility class of statics so the offset math could be shared....
There was a problem hiding this comment.
I thought of this static utility class . So something like moving this entire code into a Util class?
For instance, what if a Cell implementation had data members that cached all lengths.
This we did not do and always we restrain from doing this because say if we add those cells to memstore most of the size will go for the cell overhead. Say in a 128M flush may be 80M only is data remaining may become only overhead.
There was a problem hiding this comment.
Suggestion.
Main point is worry about exposing implementation detail especially after all the work you lot did to change the KV so we could put another impl in place (for example, one that caches sizes as they are calculated?)
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { |
There was a problem hiding this comment.
That is a spot bug inference.
|
|
||
| @Override | ||
| public boolean equals(Object other) { | ||
| return super.equals(other); |
|
|
||
| protected final ByteBuffer buf; |
There was a problem hiding this comment.
Why take away the final?
If for default constructor, pass nulls to the override?
| @@ -51,35 +53,38 @@ | |||
| */ | |||
| public static final CellComparatorImpl COMPARATOR = new CellComparatorImpl(); | |||
|
|
|||
| private static final ContiguousCellFormatComparator contiguousCellComparator = | |||
There was a problem hiding this comment.
Hmm... when would a comparator NOT be do left-to-right?
There was a problem hiding this comment.
Won't it always be a 'contiguous' left-to-right compare?
There was a problem hiding this comment.
the term Contiguous was added in terms of the Kv format. In case of cells that are encoder based it is alwasy from left to right but the format may not be KV based. Hence I went with the word 'contiguous'.
There was a problem hiding this comment.
Haven't checked but might be worth adding this note on what contiguous means to your marker interface....
| } | ||
| // "Peeling off" the most common cases where the Cells backed by KV format either onheap or | ||
| // offheap | ||
| if (a instanceof ContiguousCellFormat && b instanceof ContiguousCellFormat |
There was a problem hiding this comment.
I see. You want to use a comparator that has expectations about internals... that there methods it can call to speed up the compare.
Man. We have too many if/else's in the path. if BB, if tags, if sequenceid, if offheap.... if unsafe. If ByteBufferExtendedCell...
We don't yet have an implementation that is not contiguous?
There was a problem hiding this comment.
We have too many if/else's in the path. if BB, if tags, if sequenceid, if offheap.... if unsafe. If ByteBufferExtendedCell...
Previously we had all these branches for BytebufferExtendedcell and left being normal cell and right being normal cell etc. But we were not having the optimization of knowing the internals. Now this branching is fixed and the branches are only at this place and internally no branches. Previously the compareRows() had 4 branches, compareFamilies() had 4 branches, comparequalifiers() had 4 branches and then the tags and sequenceId. This reduces those branches. I strongly favour this lesser branching but more duplicate code
| * @return the key length | ||
| */ | ||
| int getKeyLength(); | ||
| } |
There was a problem hiding this comment.
Could this not just be internal to the Cell implementation? Does it have to be exposed like this in an Interface with special comparator?
It doesn't look like it. Has to be exposed so Comparator can make use of these methods. I seen that this patch is just generalizing the behavior done when we added BBKVComparator. Argh.
There was a problem hiding this comment.
It doesn't look like it. Has to be exposed so Comparator can make use of these methods. I seen that this patch is just generalizing the behavior done when we added BBKVComparator. Argh.
Yes I agree. I wanted to make it generic. Also the fact that if we have KV and BBKV in my cells I can get similar performance with this patch because we are having the internals with us.
| @@ -76,7 +77,7 @@ | |||
| * length and actual tag bytes length. | |||
| */ | |||
| @InterfaceAudience.Private | |||
| public class KeyValue implements ExtendedCell, Cloneable { | |||
| public class KeyValue implements ExtendedCell, ContiguousCellFormat, Cloneable { | |||
There was a problem hiding this comment.
Could the methods be added to ExtendedCell or is that different concerns?
There was a problem hiding this comment.
I think we need not add to ExtendedCell. ExtendedCell is for server to create cells. This new interface only the comparator can understand.
|
|
|
@ramkrish86 Should we close this in favor of #2776 ? |
|
Sure Stack let me close this. But before that I will ensure I read all your comments and address them in the other PR including some profiling. |
|
@ramkrish86 Did you get a chance to read through the above (and then close this)? Thanks. |
|
Closing. You can read the comments here even if closed @ramkrish86 |
Closed the original PR due to some issues with my linux/windows environment toggling. Created a new PR which can compile too.
This version of the patch tries to introduce an interface ContiguousCellFormat which understands the KV format where the data is arranged in the KV serialization format.
It tries to minimize the branching in cases of pure Kv or pure ByteBufferKV. with this patch and JMH like test with adding >100MB of data getting added to Memstore like CSLM provides >50% improvement where all the cells are pure KVs.
We did some cluster testing with only KV as the cell type and also with no DBEs. We might need some more tests to ensure we don't break anything.
In this commit apart from having the ContiguousCellComparator, We also found that the bulk load performance was slower inspite of overall improving the comparator performance by above 15%.
The reason was that PutsortReducer - get a given row with all the cells for that row and that gets written to the hfile. So effectively it is one row that is geting added to the map. Now even when cases where there are 300 cells in a row, the optimization that we expect out of ContiguousCellComparator changes does not kick in. That is due to the various branches we still have in the code and the number of cells for the optimization to kick in is still lesser.
For those cases if we can bring up the KVComparator again (currently it is deprecated - see the PutsortReducer changes in the patch) and use that KVComparator specifically for these bulk load type of cases then we are performing 15% faster than 1.3 branch. This is in line with what we are trying to do in https://issues.apache.org/jira/browse/HBASE-24754.
I can open up a discussion thread with all the details in the dev@ for others to chime in.
@anoopsjohn , @saintstack - FYI.