[GLUTEN-9456][VL] Release Velox memory manager when the reservedBytes become empty - #11249
Conversation
|
+1 |
|
cc: @j7nhai @liujiayi771 |
rui-mo
changed the title
|
Thank you, @rui-mo . It's a good summary of the memory leak issue we analyzed so far. We noted the issue in this facebookincubator/velox#8205 but it's not merged and we work around it in Gluten by adding the wait time during memory manager close. With current upstream velox, there are two issues:
What we should do and PR14722 does is to wait the current I/O request to finish and close the pending I/O request. But it may cause Prestoc++ deadlock. So PR14722 can hardly be merged into upstream. Instead we will add a factory to create customized input in Gluten which use PR14722. We temporarily picked PR14722 into gluten/velox now. With this PR we should solve all the I/O thread memleak issue totally. Let's know if you noted more failure. In theory with PR14722 we needn't the wait in the PR, we still use it for other potential memleaks. |
|
|
| bool destructed = false; | ||
| for (int32_t tryCount = 0; accumulatedWaitMs < kWaitTimeoutMs; tryCount++) { |
There was a problem hiding this comment.
@rui-mo we still need the wait time. Customer can config it and use spark.unsafe.exceptionOnMemoryLeak to detect memleaks. Without the config, the task will wait here always on real memleaks.
There was a problem hiding this comment.
Same comment here. We'd maintain a timeout so we won't hang forever by a remaining allocation in Velox.
The final goal will be removing this wait mechanism and rely on Task::requestCancel instead. So feel free to add a code comment marking this is a workaround against an Issue from Velox if the lines might confuse you already.
There was a problem hiding this comment.
The final goal will be removing this wait mechanism and rely on
Task::requestCancelinstead.
This appears to be a different solution from the ones we previously discussed. Could you provide a bit more detail? Specifically, does it cancel the async load, and should we invoke it from Gluten, or are we relying on Velox’s task cancellation?
There was a problem hiding this comment.
This appears to be a different solution from the ones we previously discussed.
Just to clarify a bit, Task::requestCancel is not a different solution but something we already called and expected it could get reliable gradually in Velox:
We don't need this wait mechanism in memory manager if Task::requestCancel works smoothly. Memory management is a sub-optimal place to determine whether the task is completely ended or not.
There was a problem hiding this comment.
does it cancel the async load
It should have them all closed as long as they are associated with the current Velox task. But apparently that's not happening so we are encountering the leak / segfault issue and now have to optimize around the memory manager destructor.
There was a problem hiding this comment.
@zhztheplayer Is this patch (facebookincubator/velox@059ffcb) intended for the purpose you mentioned? I noticed the calling of cancel would do nothing right now. If we proceed this way, it would differ a bit from importing facebookincubator/velox#14722 or customizing buffered input in Gluten. However, the fix would become more like facebookincubator/velox#8205, which releases the load through table scan's close.
There was a problem hiding this comment.
request cancel should be used for different case. In this case, the task finishes normally, not is cancelled.
8025's close() is the most elegant way to close the pending I/O request but it changes too much velox code and even hard to get merged. In theory velox has a close() for each operator which should free all resources the operator holds.
14722 relys on the class descrution to close the resource. It's easier for us to pick.
There was a problem hiding this comment.
By close / cancel (Velox PR 8205 / 14722 included) I think we are talking about the same concept - to end the computation and release the resources before the Velox task can be destructed. You can consider requestCancel a way to ensure all the Velox close APIs are called. So I guess we are already on the same page.
It was just worth noting this wait logic in ~VeloxMemoryManager is only a temporary solution for keeping the pools alive in case the operators were halted too early without completing all the async operations.
There was a problem hiding this comment.
The problem is that not all of resource related class has the close API, which should be designed initially. PR8205 is trying to solve the table scan chain but it can be hard to merged.
Waiting in ~VeloxMemoryManager here is still useful because we only solve the table scan's async IO.
@FelixYBW It seems the base branch hasn't included this PR: https://github.com/IBM/velox/tree/oss-2025_12_03. |
Oh, we pick into this branch:https://github.com/IBM/velox/commits/ibm-2025_12_03. Let's move to OAP |
5 participants
What changes are proposed in this pull request?
When the Velox memory manager is destructed, some asynchronous I/O prefetch operations may still be active, which leads to errors. The operator’s memory manager is scoped at the task level, and the error is triggered during the cleanup of task resources.
The PR addresses this by having the table scan destructor wait for all asynchronous I/O to finish and cancel any I/O operations that have not yet started. The Velox community has pointed out the possibility of a deadlock in scenarios like this, while this approach of waiting for asynchronous I/O in the table scan destructor will not cause a deadlock in Gluten, because Gluten does not use Velox’s arbitration lock.
In Gluten, the Velox memory manager also waits during the task-level resource cleanup, but the main difference is that the table scan destructor can cancel I/O operations that are no longer needed, while task-level cleanup can only wait indefinitely—even if the I/O has not started yet or is no longer relevant.
Adding a wait loop in the Velox memory manager destructor until
reservedBytes == 0partially mitigates the issue. However, a momentary state ofreservedBytes == 0does not guarantee that asynchronous threads won’t allocate memory again afterward, so this cannot fundamentally solve the problem.TODO: We can consider proposing a factory mechanism to the Velox community to allow registering custom buffered input implementations.
How was this patch tested?
Related issue: #9456