kezhuw commented on a change in pull request #15273:
URL: https://github.com/apache/flink/pull/15273#discussion_r598840667
##########
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:
For `isFree()`: Sorry for misleading. `VisibleForTesting` is sufficient.
For atomic handling placement: I am ok to either approaches. But I still
think it is a good to not spread this concurrent guarding to global.
Besides above, I am a bit curious about how to do leak detection in current
direction: `finalize` in `MemorySegment` ? Seems to me that, all these changes
might affect streaming users without much benefits. The problems are two folds:
1. `finalize` itself is a yes or no in coding phase.
2. `finalize` is applied to all `MemorySegment` no matter whether it is need
or not or active-freed.
It is a bit out of context and should belong to FLINK-21799. But I think it
is related as we might run out of choices if opt out gc-cleaner approaches and
keep only raw address directly in `MemorySegment`.
> 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.
Personally, I would prefer to keep following in long term.
* Keep concurrent guarding for unsafe memory due to bad experience for crash
and investigation.
* Configurable leak detection. Options are "none", "log" and "fail". This is
a redraw from my side in FLINK-21419. `finalize` does not meet.
--
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]
