[GLUTEN-12911][VL] Fix memory pool holding in async thread - #12919
[GLUTEN-12911][VL] Fix memory pool holding in async thread#12919FelixYBW wants to merge 3 commits into
Conversation
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
This fix looks more robust. Could you rebase the PR and let the CI be re-triggered? Not sure if the CI failure is related. Thanks.
| if (!load->loadOrFuture(&waitFuture)) { | ||
| auto& exec = folly::QueuedImmediateExecutor::instance(); | ||
| std::move(waitFuture).via(&exec).wait(); | ||
| } |
There was a problem hiding this comment.
With the barrier introduced, the above kLoading loop seems redundant. Do we need to remove it?
There was a problem hiding this comment.
Should be, but no hurt. Let's keep it there until someone can verify.
There was a problem hiding this comment.
🟡 Changes recommended
The updated header introduces direct dependencies (e.g., folly::getKeepAliveToken, std::make_unique) but doesn’t include the corresponding headers, making the build fragile due to reliance on transitive includes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses Velox backend issue #12911 by ensuring async IO load closures don’t retain MemoryPool references past GlutenDirectBufferedInput destruction, preventing pools_.size() != 0 failures during memory manager teardown.
Changes:
- Introduces an
ExecutorBarrierwrapper (owned byGlutenDirectBufferedInput) so async load enqueues can be explicitly drained at destruction time. - Updates the
GlutenDirectBufferedInputdestructor towaitAll()on the barrier (with exception swallowing/logging) after cancelling planned loads and waiting on in-flight loads. - Adjusts
clone()to pass the raw executor so clones don’t enqueue onto the original instance’s barrier.
File summaries
| File | Description |
|---|---|
| cpp/velox/memory/GlutenDirectBufferedInput.h | Wraps the IO executor with an ExecutorBarrier and waits for queued async-load closures to drain during destruction to avoid lingering MemoryPool references. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #include <glog/logging.h> | ||
|
|
||
| #include "velox/dwio/common/DirectBufferedInput.h" | ||
| #include "velox/dwio/common/ExecutorBarrier.h" |
| // gone. | ||
| if (barrier() != nullptr) { | ||
| try { | ||
| barrier()->waitAll(); |
There was a problem hiding this comment.
The task will keep waiting, will there be any issue like the task hangs there forever?
There was a problem hiding this comment.
It depends on the OS when it schedules the I/O thread. If your system can't schedule the ready thread, it means your system is already hung.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
| } catch (const std::exception& e) { | ||
| // waitAll() rethrows an exception raised by any of the loads. It must | ||
| // not escape the destructor: the loads were cancelled anyway. | ||
| LOG(WARNING) << "Async load failed while destructing GlutenDirectBufferedInput: " << e.what(); |
| #include <glog/logging.h> | ||
|
|
| } | ||
|
|
||
| folly::Executor* const rawExecutor_; | ||
| const std::unique_ptr<facebook::velox::dwio::common::ExecutorBarrier> barrier_; |
philo-he
left a comment
philo-he
left a comment
There was a problem hiding this comment.
Looks good. Thanks.
Code format issue was reported:
https://github.com/apache/gluten/actions/runs/34432544793/attempts/1#summary-102730889006
philo-he
changed the title
Solve issue #12911
Without the barrier, it's possible we mark the load as canceled, but it's never scheduled before the timeout, so it holds the memory pool pointer all the time.
The barrier will wait until the load is scheduled and exits.