MINOR: perf optimization for header serialization and type conversion#21762
MINOR: perf optimization for header serialization and type conversion#21762mjsax wants to merge 8 commits intoapache:trunkfrom
Conversation
This PR replaces the usage of ByteArrayOutputStreams with ByteBuffers. Some manual benchmarks show a perf improvement for the value-ts-header and session-header convertes of 3x.
| record.timestampType(), | ||
| record.serializedKeySize(), | ||
| record.serializedValueSize(), | ||
| recordValueWithTimestamp != null ? recordValueWithTimestamp.length : 0, |
There was a problem hiding this comment.
Stumbled over this by chance -- it's a long standing minor bug... Fixing on the side.
| record.timestampType(), | ||
| record.serializedKeySize(), | ||
| record.serializedValueSize(), | ||
| recordValueWithTimestampAndHeaders != null ? recordValueWithTimestampAndHeaders.length : 0, |
| record.timestampType(), | ||
| record.serializedKeySize(), | ||
| record.serializedValueSize(), | ||
| recordValueWithHeaders != null ? recordValueWithHeaders.length : 0, |
|
|
||
| private final RecordConverter timestampedValueConverter = rawValueToTimestampedValue(); | ||
| private final RecordConverter headersValueConverter = rawValueToHeadersValue(); | ||
| private final RecordConverter sessionValueConverter = rawValueToSessionHeadersValue(); |
There was a problem hiding this comment.
Adding missing case for session-header-store
| {50, 2, 20, 104, 101, 97, 100, 101, 114, 45, 107, 101, 121, 24, 104, 101, 97, 100, 101, | ||
| 114, 45, 118, 97, 108, 117, 101, 0, 0, 0, 0, 0, 0, 0, 10, 0}; | ||
| {50, 2, 20, 'h', 'e', 'a', 'd', 'e', 'r', '-', 'k', 'e', 'y', 24, 'h', 'e', 'a', 'd', 'e', | ||
| 'r', '-', 'v', 'a', 'l', 'u', 'e', 0, 0, 0, 0, 0, 0, 0, 10, value[0]}; |
There was a problem hiding this comment.
Found a way to make this easier to read :)
| int estimatedBufferSize = 5; | ||
| for (final Header header : headersArray) { | ||
| // adding 5 bytes for varint encoding of header-key length | ||
| estimatedBufferSize += 5 + header.key().length(); |
There was a problem hiding this comment.
we should use key.getBytes(StandardCharsets.UTF_8).length.
There was a problem hiding this comment.
It would be better to reuse the byte array from key.getBytes(StandardCharsets.UTF_8)
There was a problem hiding this comment.
Should it not be the same?
There was a problem hiding this comment.
yes, The serialized format writes UTF-8 bytes, while String.length() counts UTF-16 code units.
| */ | ||
| static ByteBuffer prepareByteBufferWithSizePrefix(final int prefix, final int bufferSize) { | ||
| final ByteBuffer varLengthBuffer = ByteBuffer.allocate(5); // 5 bytes for max varint encoding | ||
| ByteUtils.writeVarint(prefix, varLengthBuffer); |
There was a problem hiding this comment.
Would you leverage ByteUtils.sizeOfVarint to avoid creating the temporary buffer?
There was a problem hiding this comment.
Oh. I missed that we have sizeOfVarint -- I was actually considering to maybe add such a helper -- this simplifies things quite a bit.
| int estimatedBufferSize = 5; | ||
| for (final Header header : headersArray) { | ||
| // adding 5 bytes for varint encoding of header-key length | ||
| estimatedBufferSize += 5 + header.key().length(); |
There was a problem hiding this comment.
It would be better to reuse the byte array from key.getBytes(StandardCharsets.UTF_8)
|
|
||
| ByteUtils.writeVarint(headersArray.length, out); | ||
| // start with 5 bytes for varint encoding of header count | ||
| int estimatedBufferSize = 5; |
There was a problem hiding this comment.
Maybe we could use sizeOfVarint to get exact size?
int exactBufferSize = ByteUtils.sizeOfVarint(headersArray.length);
for (final Header header : headersArray) {
final String headerKey = header.key();
int headerKeySize = Utils.utf8Length(headerKey);
exactBufferSize += ByteUtils.sizeOfVarint(headerKeySize) + headerKeySize;
final byte[] value = header.value();
if (value == null) {
exactBufferSize += ByteUtils.sizeOfVarint(-1);
} else {
exactBufferSize += ByteUtils.sizeOfVarint(value.length) + value.length;
}
}
|
Thanks @nileshkumar3 @chia7712 -- pushed an update. Needed to restructure some parts of the code... |
|
|
||
| final static class PreSerializedHeaders { | ||
| final int requiredBufferSizeForHeaders; | ||
| final byte[][][] serializedHeaders; |
There was a problem hiding this comment.
This 3-dimensional array is a very bad idea... totally kills performance...
| public class HeadersSerializerTest { | ||
|
|
||
| @Test | ||
| public void test() { |
There was a problem hiding this comment.
Well, I gives me only a benchmark runtime of 20 sec (with the current code)...
| PreSerializedHeaders( | ||
| final int requiredBufferSizeForHeaders, | ||
| final byte[][] rawHeaderKeys, | ||
| final byte[][] rawHeaderValued |
3 participants
This PR replaces the usage of ByteArrayOutputStreams with ByteBuffers.
Some manual benchmarks show a perf improvement for the value-ts-header
and session-header converters of 3x.