kezhuw commented on a change in pull request #15273:
URL: https://github.com/apache/flink/pull/15273#discussion_r600180203
##########
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 have no objection for enforcing rules on memory types. The visible
difference is minor: concurrent free will fail for all memory segments.
I guess it is price of full aligned api unification for safe and unsafe
memory segment. I think we could go through current approach and see whether it
introduces much overhead for streaming users. In later stage, we could decide
whether to drop the whole things and compromise a bit in implementation or api
semantics base on concrete cases/usages/benchmarks.
Though, I still think it is a good to guard unsafe part even after all
existing invalid cases detected especially it is uncertain(FLINK-21798) that
unsafe part could be "unmanaged". It is a safety net for that uncertain future,
otherwise we are just rollback ~ rollback ~ rollback things. That is why I
think configurable is good.
--
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]
