feat: Allow flexible thread pool monitoring for Spiller cpu pool #26695
+18
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the spiller thread pool handling in PrestoServer to separately track a CPU-thread-pool executor pointer for stall monitoring while keeping a generic executor handle, and updates all server initialization and wiring to use the new pointer. Sequence diagram for PrestoServer spiller CPU pool initialization and wiringsequenceDiagram
participant PrestoServer
participant SystemConfig
participant SpillerExecutor as folly_CPUThreadPoolExecutor
participant TaskManager
PrestoServer->>SystemConfig: getSpillerNumCpuThreadsHwMultiplier()
SystemConfig-->>PrestoServer: multiplier
PrestoServer->>PrestoServer: compute numSpillerCpuThreads
alt numSpillerCpuThreads > 0
PrestoServer->>SpillerExecutor: create CPUThreadPoolExecutor(numSpillerCpuThreads, NamedThreadFactory)
PrestoServer->>PrestoServer: set spillerCpuExecutor_ = SpillerExecutor
PrestoServer->>PrestoServer: set spillerExecutor_ (std_unique_ptr_folly_Executor)
end
PrestoServer->>TaskManager: create TaskManager(driverCpuExecutor_, httpSrvCpuExecutor_, spillerCpuExecutor_)
TaskManager-->>PrestoServer: TaskManager instance
Class diagram for updated PrestoServer spiller executorsclassDiagram
class PrestoServer {
- folly_CPUThreadPoolExecutor* driverCpuExecutor_
- std_unique_ptr_folly_Executor spillerExecutor_
- folly_CPUThreadPoolExecutor* spillerCpuExecutor_
+ void initializeThreadPools()
+ void run()
+ void createTaskManager()
}
class TaskManager {
+ TaskManager(folly_CPUThreadPoolExecutor* driverCpuExecutor, folly_Executor* httpSrvCpuExecutor, folly_CPUThreadPoolExecutor* spillerCpuExecutor)
}
PrestoServer --> TaskManager : creates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- spillerCpuExecutor_ is never initialized when numSpillerCpuThreads == 0, but is still passed into TaskManager in createTaskManager(), so it should be explicitly initialized to nullptr and guarded wherever it is used.
- Having both spillerExecutor_ and spillerCpuExecutor_ pointing at the same pool introduces a risk of them getting out of sync; consider simplifying the ownership model (e.g., always using spillerExecutor_.get() where a raw pointer is needed or centralizing the cast) to avoid duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- spillerCpuExecutor_ is never initialized when numSpillerCpuThreads == 0, but is still passed into TaskManager in createTaskManager(), so it should be explicitly initialized to nullptr and guarded wherever it is used.
- Having both spillerExecutor_ and spillerCpuExecutor_ pointing at the same pool introduces a risk of them getting out of sync; consider simplifying the ownership model (e.g., always using spillerExecutor_.get() where a raw pointer is needed or centralizing the cast) to avoid duplication.
## Individual Comments
### Comment 1
<location> `presto-native-execution/presto_cpp/main/PrestoServer.cpp:548-555` </location>
<code_context>
PRESTO_STARTUP_LOG(INFO)
- << "Spiller CPU executor '" << spillerExecutor_->getName() << "', has "
- << spillerExecutor_->numThreads() << " threads.";
+ << "Spiller CPU executor '" << spillerCpuExecutor_->getName()
+ << "', has " << spillerCpuExecutor_->numThreads() << " threads.";
} else {
PRESTO_STARTUP_LOG(INFO) << "Spill executor was not configured.";
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align the null-check with the pointer being dereferenced to avoid potential null dereferences.
The guard condition checks `spillerExecutor_ != nullptr`, but the block dereferences `spillerCpuExecutor_`. If these ever get out of sync (e.g., `spillerExecutor_` set while `spillerCpuExecutor_` is left null), this will crash. Please either guard on `spillerCpuExecutor_` or ensure both pointers are checked before dereferencing `spillerCpuExecutor_`.
```suggestion
}
if (spillerCpuExecutor_ != nullptr) {
PRESTO_STARTUP_LOG(INFO)
<< "Spiller CPU executor '" << spillerCpuExecutor_->getName()
<< "', has " << spillerCpuExecutor_->numThreads() << " threads.";
} else {
PRESTO_STARTUP_LOG(INFO) << "Spill executor was not configured.";
}
auto* asyncDataCache = velox::cache::AsyncDataCache::getInstance();
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
duxiao1212
added a commit
to duxiao1212/presto
that referenced
this pull request
Nov 26, 2025
…db#26695) Summary: as title Differential Revision: D87850298
duxiao1212
force-pushed
the
export-D87850298
branch
from
a4b9441 to
68e0e7b
Compare
duxiao1212
changed the title
Summary: as title Differential Revision: D87850298
duxiao1212
force-pushed
the
export-D87850298
branch
from
68e0e7b to
a0b943f
Compare
| // folly::CPUThreadPoolExecutor. The executor is stored as abstract type to | ||
| // provide flexibility of thread pool monitoring. The underlying | ||
| // folly::CPUThreadPoolExecutor can be obtained through 'spillerCpuExecutor_'. | ||
| std::unique_ptr<folly::Executor> spillerExecutor_; |
Contributor
There was a problem hiding this comment.
Thanks @duxiao1212 for this code. I'm not sure I follow the motivation for this change. Please can you help explain.
Contributor
Author
There was a problem hiding this comment.
You're right @aditi-pandit - the original type worked. However, we need to change it to std::unique_ptr<folly::Executor> because the build system allows wrapping executors with monitoring capabilities.
Background
While investigating stuck driver issues, we identified the need for thread monitoring capability on the spiller thread executor. This helps detect and diagnose thread stalls during spill operations.
Implementation
By storing as the base type Executor, the executor can be wrapped with monitoring.
While keeping the typed spillerCpuExecutor_ pointer allows PeriodicTaskManager to access monitoring methods.
This follows the same pattern as driverExecutor_ and enables consistent thread stall detection across execution contexts.
duxiao1212
changed the title
tanjialiang
approved these changes
Dec 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: as title
Differential Revision: D87850298
Release Notes