Copilot commented on code in PR #3693:
URL: https://github.com/apache/celeborn/pull/3693#discussion_r3308582666
##########
cpp/celeborn/client/reader/WorkerPartitionReader.cpp:
##########
@@ -122,6 +136,8 @@ void WorkerPartitionReader::initAndCheck() {
shared_this->chunkQueue_.enqueue(std::move(chunk));
VLOG(1) << "WorkerPartitionReader::onSuccess: "
<< streamChunkSlice.toString();
+ destructionExecutor().add(
+ [s = std::move(shared_this)]() mutable { s.reset(); });
};
Review Comment:
`destructionExecutor().add(...)` is executed on every successful chunk
fetch, which posts a task per chunk even though the off-thread handoff is only
needed when this callback holds the last `shared_ptr` reference. This can add
noticeable scheduling overhead and keep the reader alive longer than necessary
under heavy fetch workloads. Consider only offloading when
`shared_this.use_count() == 1` (or an equivalent "last ref" check), and
otherwise let `shared_this` fall out of scope normally.
##########
cpp/celeborn/client/reader/WorkerPartitionReader.cpp:
##########
@@ -138,6 +154,8 @@ void WorkerPartitionReader::initAndCheck() {
auto exp = shared_this->exception_.wlock();
*exp = std::move(exception);
}
+ destructionExecutor().add(
+ [s = std::move(shared_this)]() mutable { s.reset(); });
};
Review Comment:
`destructionExecutor().add(...)` is invoked for every failure callback,
which will enqueue an additional task even when there are still other
`shared_ptr` owners of the reader. Since fetch failures can also be frequent
(e.g., transient network errors), consider guarding the off-thread handoff so
it only runs when this callback is about to drop the last `shared_ptr`
reference, to avoid unnecessary executor load.
--
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]
