xintongsong commented on a change in pull request #15273:
URL: https://github.com/apache/flink/pull/15273#discussion_r600131068
##########
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:
In long term, I think it would be better to rule out all of the
following cases. And by "rule out", I mean getting rid of the existing cases
and prevent new cases from being introduced.
- A segment is freed more than once (concurrently or not)
- A segment is being GC-ed without having been freed
Once the existing cases are all removed, we can strictly fail on these
cases. That also means guarding concurrent frees should not be needed. Any
concurrent frees should have already failed due to multiple frees during CI
tests.
The benefit of enforcing these rules on memory types that it simplifies the
contract of using a memory segment and improves maintainability. Otherwise, it
would be complicated to understand that a segment has to be freed exactly once
if its unsafe, while it's okay to free another segment multiple times (or
never) if it's heap/direct. Users of `MemorySegment` should not be forced to
understand the underlying memory type.
I admit enforcing such rules comes at a price of extra overhead in
freeing/finalizing the segments. However, I'd argue that the price is
insignificant compared to the benefits.
--
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
