Copilot commented on code in PR #12919:
URL: https://github.com/apache/gluten/pull/12919#discussion_r3975188203
##########
cpp/velox/memory/GlutenDirectBufferedInput.h:
##########
@@ -66,11 +118,28 @@ class GlutenDirectBufferedInput : public
facebook::velox::dwio::common::DirectBu
}
}
coalescedLoads_.clear();
+ // The cancelled loads above are still referenced by the AsyncLoadHolder
+ // closures queued on the executor, and those closures hold a shared_ptr on
+ // the memory pool. Wait until the executor has run and destroyed them so
+ // that the pool reference is released before this destructor returns,
+ // instead of on an IO thread after the task and its memory manager are
+ // gone.
+ if (barrier() != nullptr) {
+ try {
+ barrier()->waitAll();
+ } 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();
Review Comment:
`~GlutenDirectBufferedInput()` is implicitly `noexcept(true)`, so any
exception type not derived from `std::exception` escaping `waitAll()` will
still call `std::terminate`. To guarantee the destructor can’t terminate the
process, catch `...` as well (optionally logging an “unknown exception”
message) or otherwise ensure all possible throws are covered.
##########
cpp/velox/memory/GlutenDirectBufferedInput.h:
##########
@@ -17,11 +17,59 @@
#pragma once
+#include <glog/logging.h>
+
#include "velox/dwio/common/DirectBufferedInput.h"
+#include "velox/dwio/common/ExecutorBarrier.h"
namespace gluten {
-class GlutenDirectBufferedInput : public
facebook::velox::dwio::common::DirectBufferedInput {
+namespace detail {
+
+// Owns the ExecutorBarrier that wraps the IO executor passed to
+// DirectBufferedInput. DirectBufferedInput::readRegions() enqueues an
+// AsyncLoadHolder closure per planned load, and that closure keeps a
+// shared_ptr on the reader's MemoryPool. Cancelling a load only flips its
+// state, it does not dequeue or destroy the closure, so the pool reference
+// survives until the executor happens to drain that entry. Routing all the
+// enqueues through a barrier lets the destructor wait for them explicitly.
+//
+// This must be listed as a base before DirectBufferedInput so that it is
+// constructed first (the barrier pointer is handed to the base constructor)
+// and destructed last (the barrier has to outlive any closure referring to
+// it).
+class ExecutorBarrierHolder {
+ public:
+ explicit ExecutorBarrierHolder(folly::Executor* executor)
+ : rawExecutor_(executor), barrier_(makeBarrier(executor)) {}
+
+ protected:
+ // The unwrapped executor, to be handed to clones instead of this object's
+ // barrier.
+ folly::Executor* rawExecutor() const {
+ return rawExecutor_;
+ }
+
+ facebook::velox::dwio::common::ExecutorBarrier* barrier() const {
+ return barrier_.get();
+ }
+
+ private:
+ static std::unique_ptr<facebook::velox::dwio::common::ExecutorBarrier>
makeBarrier(folly::Executor* executor) {
+ if (executor == nullptr) {
+ return nullptr;
+ }
+ return
std::make_unique<facebook::velox::dwio::common::ExecutorBarrier>(folly::getKeepAliveToken(executor));
+ }
+
+ folly::Executor* const rawExecutor_;
+ const std::unique_ptr<facebook::velox::dwio::common::ExecutorBarrier>
barrier_;
Review Comment:
Making the `std::unique_ptr` itself `const` is unusual and can unnecessarily
restrict future refactors (e.g., move-assigning, resetting, or conditionally
reinitializing the barrier). If immutability is desired, prefer keeping the
pointer non-const and only exposing const-correct access (as you already do via
`barrier()`), which preserves flexibility without changing behavior.
##########
cpp/velox/memory/GlutenDirectBufferedInput.h:
##########
@@ -17,11 +17,59 @@
#pragma once
+#include <glog/logging.h>
+
Review Comment:
Including `glog` in a public header introduces a new transitive dependency
for every translation unit that includes this header, which can break builds
(if `glog` isn’t available everywhere) and increase compile time. Consider
switching to the project’s preferred logging facility that’s already commonly
included (e.g., Velox/Folly logging wrappers), or moving the destructor (and
its logging) out of the header into a `.cc` file to keep `glog` out of the
public interface.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
