Implement basic logic of re-using buffers #382
Conversation
Codecov Report
@@ Coverage Diff @@
## master #382 +/- ##
============================================
+ Coverage 61.84% 62.37% +0.52%
- Complexity 1191 1248 +57
============================================
Files 131 135 +4
Lines 6401 6631 +230
Branches 475 516 +41
============================================
+ Hits 3959 4136 +177
- Misses 2175 2219 +44
- Partials 267 276 +9
Continue to review full report at Codecov.
|
| public BinarySerializer(BuffedWriter writer, boolean enableCompress) { | ||
| this.enableCompress = enableCompress; | ||
| BuffedWriter compressWriter = null; | ||
| if (enableCompress) { | ||
| compressWriter = new CompressedBuffedWriter(ClickHouseDefines.SOCKET_SEND_BUFFER_BYTES, writer); | ||
| } | ||
| switcher = new Switcher<>(compressWriter, writer); | ||
| // max num of byte is 8 for double and long | ||
| writeBuffer = new byte[8]; |
|
First of all, thanks a lot for addressing this over-allocation problem. I was contemplating a simpler change, but this one is a lot better!
|
|
@AndreyNudko Please see comment in linked ticket. I am not the right person to ask those questions and not sure the right person will have more time to work on this to address those. I am but a messenger. |
|
@akamensky Don't get me wrong - those questions are more for maintainers to consider, I'm certainly not asking for more from you guys. And I really appreciate the effort you made. |
|
@akamensky Thanks for the pr, I'll take time to review this.
I do think letting it be the way before is better, users can modify set these to other values.
Yes, that's the side effect, manually re-using the buffer or let them be controlled by JVM. But I think it may be worthy to do if we can set fixed blocksize (batchsize) for each insert( better let the value lesser than 1048576). clickhouse-go seems to have a buffer block in each connection, see: https://github.com/ClickHouse/clickhouse-go/blob/master/clickhouse.go#L46-L61 |
| } | ||
|
|
||
| public void recycleBuffer(ColumnWriterBuffer buffer) { | ||
| stack.addLast(buffer); |
There was a problem hiding this comment.
how about reset it before addLast, making stack keeps all clean buffers.
| */ | ||
| public class ColumnWriterBufferFactory { | ||
|
|
||
| private final ConcurrentLinkedDeque<ColumnWriterBuffer> stack = new ConcurrentLinkedDeque<>(); |
There was a problem hiding this comment.
I think it's better to export a method named clear to clear all buffers manually.
|
@sundy-li as I mentioned, please check the comment in linked ticket. I did not write those changes, it comes from a colleague who is a Java expert. They do not have more time to invest into this changeset. They also mentioned that the implementation is very crude, so you may be right on those comments you left. I am not the right person to follow up on those comments, but feel free to modify these changes as you see appropriate. Meantime we are using those changes internally and it reduced CPU load dramatically. |
* Implement basic logic of re-using buffers * Add clearAllBuffers method inside ColumnWriterBufferFactory * Tab to spaces Co-authored-by: sundyli <543950155@qq.com>
…usepower#382 Instead of copying serialized data from ByteBuffer via intermediate byte[] array, just write backing array directly (this somewhat couples ColumnWriterBuffer with ByteArrayWriter internals, however as the name implies ByteArrayWriter is expected to use heap buffers). ByteArrayWriter and CompressedBuffedWriter use fixed-size buffers internally and split bytes as required anyway SocketBuffedWriter is updated to write in fixed-size chunks to minimize allocations in the native code; this may be dubious optimisation, but in worst case it should be harmless
Closes #353