Optimize validity buffer concat. #2626
Conversation
|
cc @jlowe I did some benchmark and didn't notice much performance improvement. |
Signed-off-by: liurenjie1024 <[email protected]>
liurenjie1024
force-pushed
the
ray/kudo-opt-concat
branch
from
04d78bd to
3cff316
Compare
|
build |
| // Extract appendCount bits from srcByte, starting from curSrcBitIdx | ||
| byte mask = (byte) (((1 << appendCount) - 1) & 0xFF); | ||
| srcByte = (byte) ((srcByte >>> curSrcBitIdx) & mask); | ||
| int totalRowCount = toIntExact(sliceInfo.getRowCount() + sliceInfo.getValidityBufferInfo().getBeginBit()); |
There was a problem hiding this comment.
The name of this variable makes reading this confusing. It's not a total row count as the name implies. It's the end index or ending row, IIUC.
| // Sets the bits in destination buffer starting from curDestBitIdx to 0 | ||
| byte destByte = dest.getByte(curDestByteIdx); | ||
| destByte = (byte) (destByte & ((1 << curDestBitIdx) - 1)); | ||
| if (dest.getLength() >= (curDestOffset + Integer.BYTES)) { |
There was a problem hiding this comment.
Conditionals should not be in the body of the while loop. The while loop should be as simple as possible, since that's expected to be the hotspot. IMO the code should be structured into three parts similar to the following:
if (curSrcIdx % 8 != 0) {
// read an int from the buffer
// mask off the unused bits
// count the bits
// shift and store the bits
}
while (whole_ints_left_in_buffer) {
// read int from buffer
// count bits
// shift and store the bits
}
if (leftover bits) {
// read an int from the buffer (leverage padded buffer here)
// mask off the unused bits
// count the bits
// shift and store the bits
}
| byte destByte = dest.getByte(curDestByteIdx); | ||
| destByte = (byte) (destByte & ((1 << curDestBitIdx) - 1)); | ||
| if (dest.getLength() >= (curDestOffset + Integer.BYTES)) { | ||
| // We have enough room to get an int |
There was a problem hiding this comment.
Don't we always have enough space to get an integer from the destination buffer because it's at least 4-byte padded?
| byte[] destBytes = new byte[4]; | ||
| dest.getBytes(destBytes, 0, curDestOffset, destBufRemBytes); | ||
| int destInt = ByteBuffer.wrap(destBytes).order(ByteOrder.LITTLE_ENDIAN).getInt(); |
There was a problem hiding this comment.
Curious why we're doing byte-at-time here and endian stuff when we don't up above? This is still grabbing 4 bytes like the above code. Byte-at-a-time is usually a lot slower, might be faster to read the int from the buffer (we're loading 4 bytes anyway) and call Integer.reverseBytes (a HotSpot intrinsic candidate) if ByteOrder.nativeOrder == ByteOrder.LITTLE_ENDIAN.
| ByteBuffer.wrap(destBytes).order(ByteOrder.LITTLE_ENDIAN).putInt(destInt); | ||
| dest.setBytes(curDestOffset, destBytes, 0, destBufRemBytes); |
There was a problem hiding this comment.
Similar to above, can call setInt here and leverage ByteOrder to determine when we need to swap bytes or not.
|
Please update the PR description. It will be displayed in the commit log. Simply saying "closes XXX" will just show that words, which is difficult to track the changes through commit log. |
To be fair we do not have the automation (yet) of checking in PRs using the PR description. I follow the convention of copying the PR description as the commit message but it's not mandated and not followed widely in NVIDIA/spark* repos. |
|
Could we copy by long instead of int? We can avoid to use |
| // Sets the bits in destination buffer starting from curDestBitIdx to 0 | ||
| byte destByte = dest.getByte(curDestByteIdx); | ||
| destByte = (byte) (destByte & ((1 << curDestBitIdx) - 1) & 0xFF); | ||
| while (curSrcIdx < totalRowCount) { |
There was a problem hiding this comment.
So does this concatenate the validity buffers, one word (32 bits) at a time, in a serial manner, in Java?
Can't we use C++/CUDA for accelerating this?
Close #2579