Optimize pipe request serialization buffer sizing#18233
Conversation
44dc635 to
793b0a6
Compare
jt2594838
left a comment
jt2594838
left a comment
There was a problem hiding this comment.
May add some performance results.
|
|
||
| static int calculateSerializedSize( | ||
| final List<ByteBuffer> insertNodeBuffers, final List<ByteBuffer> tabletBuffers) { | ||
| return Integer.BYTES * 3 |
There was a problem hiding this comment.
Add some comments to explain what the 3 is. The same below.
| static int calculateSerializedSize( | ||
| final List<ByteBuffer> insertNodeBuffers, final List<ByteBuffer> tabletBuffers) { | ||
| return Integer.BYTES * 3 | ||
| + insertNodeBuffers.stream().mapToInt(ByteBuffer::limit).sum() | ||
| + tabletBuffers.stream().mapToInt(ByteBuffer::limit).sum(); | ||
| } |
There was a problem hiding this comment.
Better to use limit - position.
Where are the sizes of the buffers recorded? The sizes do not seem to be counted in the size.
| private static int serializedStringSize(final String value) { | ||
| return Integer.BYTES + (value == null ? 0 : value.getBytes(TSFileConfig.STRING_CHARSET).length); | ||
| } |
| req.body = insertNode.serializeToByteBuffer(); | ||
| try (final PublicBAOS byteArrayOutputStream = | ||
| new PublicBAOS(calculateSerializedSize(insertNode)); | ||
| final DataOutputStream outputStream = new DataOutputStream(byteArrayOutputStream)) { | ||
| insertNode.serialize(outputStream); | ||
| req.body = ByteBuffer.wrap(byteArrayOutputStream.getBuf(), 0, byteArrayOutputStream.size()); | ||
| } catch (final IOException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
May pass the size to serializeToByteBuffer().
It would be even better to let the InsertNode handle this itself.
| @Override | ||
| protected int serializedAttributesSize() { | ||
| int size = PlanNodeType.BYTES + Integer.BYTES; | ||
| for (final InsertTabletNode insertTabletNode : insertTabletNodeList) { | ||
| size += insertTabletNode.baseSubSerializedSizeForPipe(); | ||
| } | ||
| return size + parentInsertTabletNodeIndexList.size() * Integer.BYTES; | ||
| } |
There was a problem hiding this comment.
It feels weird that this method does not seem to be made for Pipe exclusively, but it calls a method that seems to be.
Check the consistency of semantics and rename methods accordingly.
| switch (dataType) { | ||
| case BOOLEAN: | ||
| return Byte.BYTES + Byte.BYTES; | ||
| case INT32: | ||
| case DATE: | ||
| return Byte.BYTES + Integer.BYTES; | ||
| case INT64: | ||
| case TIMESTAMP: | ||
| return Byte.BYTES + Long.BYTES; | ||
| case FLOAT: | ||
| return Byte.BYTES + Float.BYTES; | ||
| case DOUBLE: | ||
| return Byte.BYTES + Double.BYTES; | ||
| case TEXT: | ||
| case STRING: | ||
| case BLOB: | ||
| case OBJECT: | ||
| return Byte.BYTES + ReadWriteIOUtils.sizeToWrite((Binary) values[index]); | ||
| default: | ||
| throw new UnSupportedDataTypeException(UNSUPPORTED_DATA_TYPE + dataType); | ||
| } |
There was a problem hiding this comment.
Use enhanced-switch and explicitly list all branches.
There was a problem hiding this comment.
Or use org.apache.tsfile.enums.TSDataType#getDataTypeSize
There was a problem hiding this comment.
May consider caching the serialized size to avoid duplicated calculation when serialization is called multiple times.
| size += Integer.BYTES; | ||
| size += Byte.BYTES; |
There was a problem hiding this comment.
Add some comments for these. The same below.
2 participants
Description
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR