You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "shizy818 (via GitHub)" <gi...@apache.org> on 2025/12/19 08:14:38 UTC
[PR] fix: memory leak during tvlist owner transfer scenario [iotdb]
shizy818 opened a new pull request, #16932:
URL: https://github.com/apache/iotdb/pull/16932
## Description
Steps to reproduce the issue:
1. During flushing, the TVList is accessed by Query 1. FragmentInstanceContext of Query 1 calls `reserveMemoryCumulatively` to reserve memory. Query 1 is added into TVList's query set and be set as owner.
2. Query 2 access the TVList as well and is added into TVList's query set.
3. When Query 1 terminates, it should transfers the ownership of the TVList to Query 2. FragmentInstanceContext of Query 1 calls `releaseMemoryReservationManager` to release memory.
4. When Query 2 terminates, FragmentInstanceContext calls `reserveMemoryCumulatively` to release memory because Query 2 is the last query and be the owner.
Release operation is invoked twice. This PR is to transfer momery virtually from MemoryReservationManager of Query 1 to that of Query 2 without actually allocating/freeing the memory.
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: memory leak during tvlist owner transfer scenario [iotdb]
Posted by "JackieTien97 (via GitHub)" <gi...@apache.org>.
JackieTien97 merged PR #16932:
URL: https://github.com/apache/iotdb/pull/16932
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: memory leak during tvlist owner transfer scenario [iotdb]
Posted by "shizy818 (via GitHub)" <gi...@apache.org>.
shizy818 commented on code in PR #16932:
URL: https://github.com/apache/iotdb/pull/16932#discussion_r2634338040
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/MemoryReservationManager.java:
##########
@@ -43,4 +45,23 @@ public interface MemoryReservationManager {
* this manager ends, Or the memory to be released in the batch may not be released correctly.
*/
void releaseAllReservedMemory();
+
+ /**
+ * Release memory virtually without actually freeing the memory. This is used for memory
+ * reservation transfer scenarios where memory ownership needs to be transferred between different
+ * FragmentInstances without actual memory deallocation.
+ *
+ * <p>NOTE: When calling this method, it should be guaranteed that bytesToBeReserved +
+ * reservedBytesInTotal >= size to ensure proper memory accounting and prevent negative
+ * reservation values.
+ */
+ Pair<Long, Long> releaseMemoryVirtually(final long size);
+
+ /**
+ * Reserve memory virtually without actually allocating new memory. This is used to transfer
+ * memory ownership from one FragmentInstances to another by reserving the memory that was
+ * previously released virtually. It updates the internal reservation state without changing the
+ * actual memory allocation.
+ */
+ void reserveMemoryVirtually(final long releasedBytesInReserved, final long releasedBytesInTotal);
Review Comment:
Done. I named the parameters using AI...
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: memory leak during tvlist owner transfer scenario [iotdb]
Posted by "shizy818 (via GitHub)" <gi...@apache.org>.
shizy818 commented on code in PR #16932:
URL: https://github.com/apache/iotdb/pull/16932#discussion_r2634334980
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/NotThreadSafeMemoryReservationManager.java:
##########
@@ -91,4 +93,25 @@ public void releaseAllReservedMemory() {
bytesToBeReleased = 0;
}
}
+
+ @Override
+ public Pair<Long, Long> releaseMemoryVirtually(final long size) {
+ if (bytesToBeReserved >= size) {
+ bytesToBeReserved -= size;
+ return new Pair<>(size, 0L);
+ } else {
+ long releasedBytesInReserved = bytesToBeReserved;
+ long releasedBytesInTotal = size - bytesToBeReserved;
+ bytesToBeReserved = 0;
+ reservedBytesInTotal -= releasedBytesInTotal;
+ return new Pair<>(releasedBytesInReserved, releasedBytesInTotal);
+ }
+ }
+
+ @Override
+ public void reserveMemoryVirtually(
+ final long releasedBytesInReserved, final long releasedBytesInTotal) {
+ bytesToBeReserved += releasedBytesInReserved;
+ reservedBytesInTotal += releasedBytesInTotal;
Review Comment:
done
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: memory leak during tvlist owner transfer scenario [iotdb]
Posted by "JackieTien97 (via GitHub)" <gi...@apache.org>.
JackieTien97 commented on code in PR #16932:
URL: https://github.com/apache/iotdb/pull/16932#discussion_r2634206864
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/NotThreadSafeMemoryReservationManager.java:
##########
@@ -91,4 +93,25 @@ public void releaseAllReservedMemory() {
bytesToBeReleased = 0;
}
}
+
+ @Override
+ public Pair<Long, Long> releaseMemoryVirtually(final long size) {
+ if (bytesToBeReserved >= size) {
+ bytesToBeReserved -= size;
+ return new Pair<>(size, 0L);
+ } else {
+ long releasedBytesInReserved = bytesToBeReserved;
+ long releasedBytesInTotal = size - bytesToBeReserved;
+ bytesToBeReserved = 0;
+ reservedBytesInTotal -= releasedBytesInTotal;
+ return new Pair<>(releasedBytesInReserved, releasedBytesInTotal);
+ }
+ }
+
+ @Override
+ public void reserveMemoryVirtually(
+ final long releasedBytesInReserved, final long releasedBytesInTotal) {
+ bytesToBeReserved += releasedBytesInReserved;
+ reservedBytesInTotal += releasedBytesInTotal;
Review Comment:
```suggestion
reservedBytesInTotal += releasedBytesInTotal;
reserveMemoryCumulatively(releasedBytesInReserved);
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/MemoryReservationManager.java:
##########
@@ -43,4 +45,23 @@ public interface MemoryReservationManager {
* this manager ends, Or the memory to be released in the batch may not be released correctly.
*/
void releaseAllReservedMemory();
+
+ /**
+ * Release memory virtually without actually freeing the memory. This is used for memory
+ * reservation transfer scenarios where memory ownership needs to be transferred between different
+ * FragmentInstances without actual memory deallocation.
+ *
+ * <p>NOTE: When calling this method, it should be guaranteed that bytesToBeReserved +
+ * reservedBytesInTotal >= size to ensure proper memory accounting and prevent negative
+ * reservation values.
+ */
+ Pair<Long, Long> releaseMemoryVirtually(final long size);
+
+ /**
+ * Reserve memory virtually without actually allocating new memory. This is used to transfer
+ * memory ownership from one FragmentInstances to another by reserving the memory that was
+ * previously released virtually. It updates the internal reservation state without changing the
+ * actual memory allocation.
+ */
+ void reserveMemoryVirtually(final long releasedBytesInReserved, final long releasedBytesInTotal);
Review Comment:
```suggestion
void reserveMemoryVirtually(final long bytesToBeReserved, final long bytesAlreadyReserved);
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/FakedMemoryReservationManager.java:
##########
@@ -32,4 +34,13 @@ public void releaseMemoryCumulatively(long size) {}
@Override
public void releaseAllReservedMemory() {}
+
+ @Override
+ public Pair<Long, Long> releaseMemoryVirtually(final long size) {
+ return null;
Review Comment:
return new Pair<>(0, 0)? Otherwise, it may cause NPE.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/MemoryReservationManager.java:
##########
@@ -43,4 +45,23 @@ public interface MemoryReservationManager {
* this manager ends, Or the memory to be released in the batch may not be released correctly.
*/
void releaseAllReservedMemory();
+
+ /**
+ * Release memory virtually without actually freeing the memory. This is used for memory
+ * reservation transfer scenarios where memory ownership needs to be transferred between different
+ * FragmentInstances without actual memory deallocation.
+ *
+ * <p>NOTE: When calling this method, it should be guaranteed that bytesToBeReserved +
+ * reservedBytesInTotal >= size to ensure proper memory accounting and prevent negative
+ * reservation values.
+ */
+ Pair<Long, Long> releaseMemoryVirtually(final long size);
Review Comment:
add some java doc about the return value
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/MemoryReservationManager.java:
##########
@@ -43,4 +45,23 @@ public interface MemoryReservationManager {
* this manager ends, Or the memory to be released in the batch may not be released correctly.
*/
void releaseAllReservedMemory();
+
+ /**
+ * Release memory virtually without actually freeing the memory. This is used for memory
+ * reservation transfer scenarios where memory ownership needs to be transferred between different
+ * FragmentInstances without actual memory deallocation.
+ *
+ * <p>NOTE: When calling this method, it should be guaranteed that bytesToBeReserved +
+ * reservedBytesInTotal >= size to ensure proper memory accounting and prevent negative
+ * reservation values.
+ */
+ Pair<Long, Long> releaseMemoryVirtually(final long size);
+
+ /**
+ * Reserve memory virtually without actually allocating new memory. This is used to transfer
+ * memory ownership from one FragmentInstances to another by reserving the memory that was
+ * previously released virtually. It updates the internal reservation state without changing the
+ * actual memory allocation.
+ */
+ void reserveMemoryVirtually(final long releasedBytesInReserved, final long releasedBytesInTotal);
Review Comment:
add some java doc about these two parameters
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: memory leak during tvlist owner transfer scenario [iotdb]
Posted by "shizy818 (via GitHub)" <gi...@apache.org>.
shizy818 commented on code in PR #16932:
URL: https://github.com/apache/iotdb/pull/16932#discussion_r2634335787
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/MemoryReservationManager.java:
##########
@@ -43,4 +45,23 @@ public interface MemoryReservationManager {
* this manager ends, Or the memory to be released in the batch may not be released correctly.
*/
void releaseAllReservedMemory();
+
+ /**
+ * Release memory virtually without actually freeing the memory. This is used for memory
+ * reservation transfer scenarios where memory ownership needs to be transferred between different
+ * FragmentInstances without actual memory deallocation.
+ *
+ * <p>NOTE: When calling this method, it should be guaranteed that bytesToBeReserved +
+ * reservedBytesInTotal >= size to ensure proper memory accounting and prevent negative
+ * reservation values.
+ */
+ Pair<Long, Long> releaseMemoryVirtually(final long size);
+
+ /**
+ * Reserve memory virtually without actually allocating new memory. This is used to transfer
+ * memory ownership from one FragmentInstances to another by reserving the memory that was
+ * previously released virtually. It updates the internal reservation state without changing the
+ * actual memory allocation.
+ */
+ void reserveMemoryVirtually(final long releasedBytesInReserved, final long releasedBytesInTotal);
Review Comment:
done
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/MemoryReservationManager.java:
##########
@@ -43,4 +45,23 @@ public interface MemoryReservationManager {
* this manager ends, Or the memory to be released in the batch may not be released correctly.
*/
void releaseAllReservedMemory();
+
+ /**
+ * Release memory virtually without actually freeing the memory. This is used for memory
+ * reservation transfer scenarios where memory ownership needs to be transferred between different
+ * FragmentInstances without actual memory deallocation.
+ *
+ * <p>NOTE: When calling this method, it should be guaranteed that bytesToBeReserved +
+ * reservedBytesInTotal >= size to ensure proper memory accounting and prevent negative
+ * reservation values.
+ */
+ Pair<Long, Long> releaseMemoryVirtually(final long size);
Review Comment:
done
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/planner/memory/FakedMemoryReservationManager.java:
##########
@@ -32,4 +34,13 @@ public void releaseMemoryCumulatively(long size) {}
@Override
public void releaseAllReservedMemory() {}
+
+ @Override
+ public Pair<Long, Long> releaseMemoryVirtually(final long size) {
+ return null;
Review Comment:
done
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org