pnowojski commented on a change in pull request #6833: [FLINK-10537][network]
Fix network small performance degradation after merging [FLINK-9913]
URL: https://github.com/apache/flink/pull/6833#discussion_r225459139
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/api/serialization/SpanningRecordSerializer.java
##########
@@ -127,6 +116,7 @@ public void reset() {
@Override
public void prune() {
+ serializationBuffer.clear();
Review comment:
Yes, I know that it will be called and that's not a bug fix. I've added this
line when I was going though your PR trying to find any differences in
behaviour vs the old code. I didn't expect this to be a performance issue and I
still don't. However when I was reverting the changes I decided to keep it.
From the consistency point of view it seems better to call clear
(`this.position = 0;`) before/during pruning the buffer
(`serializationBuffer.pruneBuffer();`). Otherwise for brief moment of time when
we exit this method, we might have `position` pointing to some non existing
place (potential `ArrayIndexOutOfBound`.
Maybe `serializationBuffer .pruneBuffer` should be setting the position to
0? Or maybe I should split this change to separate commit. As it is now, it
suggests that it has something to do with performance (which I doubt).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
