[GitHub] pnowojski commented on a change in pull request #6833: [FLINK-10537][network] Fix network small performance degradation after merging [FLINK-9913]

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org


With regards,
Apache Git Services