akalash commented on a change in pull request #15885:
URL: https://github.com/apache/flink/pull/15885#discussion_r633578223
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/buffer/BufferBuilder.java
##########
@@ -153,7 +153,12 @@ public BufferRecycler getRecycler() {
}
public void recycle() {
- recycler.recycle(memorySegment);
+ // If at least one consumer was created then they responsible for the
memory recycling
+ // because BufferBuilder doesn't contain a references counter so it
will be impossible to
+ // correctly recycle memory here.
+ if (!bufferConsumerCreated) {
+ recycler.recycle(memorySegment);
+ }
Review comment:
Yes, it is still definitely a hack. In my opinion, the right solution is
to avoid direct writing into `MemorySegment` from `BufferBuilder`. It means we
just should change the implementation of `BufferBuilder` in such a way that
using `Buffer` instead of `MemorySegment`. As I understand now you don't have
any objections about such a solution if the benchmark doesn't show any
degradation?
In any case, some answers to the questions:
- The contract is simple - `BufferBuilder#recycle()` should be called always
when `BufferBuilder` is not needed anymore. You don't need to think
`BufferConsumer` was created or not.
- In general, renaming `recycle()` to `close()` makes sense to me since
`BufferBuilder` doesn't have `retain` method and ideally, should be closed
after usage.(we can think about it when we will agree on a final solution)
- There are a couple of problems still not resolved - writing to already
released `memorySegment` or creating 'BufferConsumer' from already closed
'BufferBuilder'. They both can be resolved by solution which we already
discussed(using `Buffer` instead of `memorySegment` inside of `BufferBuilder`)
--
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:
[email protected]
