xintongsong commented on a change in pull request #15273:
URL: https://github.com/apache/flink/pull/15273#discussion_r598543460
##########
File path:
flink-core/src/main/java/org/apache/flink/core/memory/MemorySegment.java
##########
@@ -217,10 +222,14 @@ public int size() {
/**
* Checks whether the memory segment was freed.
*
+ * <p>This method internally involves cross-thread synchronization. Do not
use for performance
+ * sensitive code paths.
+ *
* @return <tt>true</tt>, if the memory segment has been freed,
<tt>false</tt> otherwise.
*/
public boolean isFreed() {
- return address > addressLimit;
+ // in performance sensitive cases, use 'address > addressLimit' instead
+ return isFreedAtomic.get();
Review comment:
I agree that `isFree()` does not guarantee success of later access
anyway. I'd keep it untouched. I'd keep it public for the testing purpose.
As for the atomic handling, I'd prefer to keep it as is. Network stack is
performance sensitive because many of the operations are on per-record code
paths. However, as far as I checked, freeing of memory segments are not on
per-record paths. (And if they are, that probably indicates
`FreeingBufferRecycler` should not be used.)
Moreover, once we have hunted down all the multiple-free cases, we can
enforce a strict failing against multiple-frees. At that time, we won't need
this atomic checking anymore. Therefore, I'm in favor of not introduce the
complexity of an enriched cleaner struct, which will soon be removed.
--
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:
us...@infra.apache.org
