[GitHub] [flink] pnowojski commented on a change in pull request #12353: [FLINK-17322][network] Fixes BroadcastRecordWriter overwriting memory segments on first finished BufferConsumer.

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

pnowojski commented on a change in pull request #12353:
URL: https://github.com/apache/flink/pull/12353#discussion_r432932457



##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/api/writer/BroadcastRecordWriter.java
##########
@@ -130,7 +132,7 @@ public BufferBuilder requestNewBufferBuilder(int targetChannel) throws IOExcepti
 
 		BufferBuilder builder = super.requestNewBufferBuilder(targetChannel);
 		if (randomTriggered) {
-			addBufferConsumer(builder.createBufferConsumer(), targetChannel);
+			addBufferConsumer(randomConsumer = builder.createBufferConsumer(), targetChannel);

Review comment:
       Creating `Buffer` in `BufferBuilder` would require two more `volatile` access when bumping the reference count on the per buffer path. I would expect this to be visible in our benchmarks with high number of network channels and frequent flushing (`1000,1ms`). I'm not sure, I might be wrong but I think I was even benchmarking this on Nico's request during the initial development of low latency changes. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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