kezhuw commented on a change in pull request #15273:
URL: https://github.com/apache/flink/pull/15273#discussion_r597595319
##########
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:
Taking possible concurrent accessing into account, `isFree()` is always
best effort that says later access is not guaranteed to success. I pointed this
out in
[#15246](https://github.com/apache/flink/pull/15246#discussion_r595425459). I
suggest to make it private and keep it untouched.
For whether to guard all segments freeings, I'd prefer to only unsafe part,
but it is ok to go other direction. Here is my considerations:
* `MemorySegment.free` is not only used by `MemoryManager` but also
`FreeingBufferRecycler` which is spread over network stack. In there, a
volatile access is counted-:). See `AvailabilityProvider.isAvailable`.
* For safe part, it is far less destructive comparing to unsafe part. I
think this "destructive" motivate us for this protection, at least for me.
All for all, we are guarding freeing not accessing.
--
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]
