refactor: Migrate to folly::available_concurrency#27328
refactor: Migrate to folly::available_concurrency#27328aditi-pandit merged 1 commit intoprestodb:masterfrom
Conversation
Summary: The name `hardware_concurrency`, while parallel to `std::thread::hardware_concurrency`, may be misleading. Migrate to the name `available_concurrency`. Differential Revision: D96480646
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors Presto server configuration and tests to use folly::available_concurrency instead of folly::hardware_concurrency wherever CPU/thread-capacity is derived, keeping behavior but aligning naming with Folly’s available concurrency concept. Flow diagram for thread pool sizing with folly_available_concurrencyflowchart TD
A[Start PrestoServer_initializeThreadPools] --> B[Call folly_available_concurrency]
B --> C[Store result in hwConcurrency]
C --> D[Read SystemConfig_exchangeHttpClientNumIoThreadsHwMultiplier]
D --> E["Compute numExchangeHttpClientIoThreads = max(multiplier * folly_available_concurrency, 1)"]
E --> F[Create folly_IOThreadPoolExecutor for exchangeHttpIoExecutor_]
C --> G[Read SystemConfig_exchangeHttpClientNumCpuThreadsHwMultiplier]
G --> H["Compute numExchangeHttpClientCpuThreads = max(multiplier * folly_available_concurrency, 1)"]
H --> I[Create folly_CPUThreadPoolExecutor for exchangeHttpCpuExecutor_]
C --> J[Read SystemConfig_connectorNumCpuThreadsHwMultiplier]
J --> K["Compute numConnectorCpuThreads = max(multiplier * folly_available_concurrency, 0)"]
K --> L{numConnectorCpuThreads > 0?}
L -- Yes --> M[Create folly_CPUThreadPoolExecutor for connectorCpuExecutor_]
L -- No --> N[Skip CPU connector executor]
C --> O[Read SystemConfig_connectorNumIoThreadsHwMultiplier]
O --> P["Compute numConnectorIoThreads = max(multiplier * folly_available_concurrency, 0)"]
P --> Q{numConnectorIoThreads > 0?}
Q -- Yes --> R[Create folly_IOThreadPoolExecutor for connectorIoExecutor_]
Q -- No --> S[Skip IO connector executor]
M --> T[Finish PrestoServer_initializeThreadPools]
N --> T
R --> T
S --> T
subgraph Overload_check_and_status_reporting
U[PrestoServer_checkOverload] --> V[static hwConcurrency = folly_available_concurrency]
V --> W["Compute overloadedThresholdQueuedDrivers = hwConcurrency * workerOverloadedThresholdQueuedMultiplier"]
X[PrestoServer_fetchNodeStatus] --> Y[Call folly_available_concurrency]
Y --> Z[Include availableConcurrency in NodeStatus]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The helper
hardwareConcurrency()inConfigs.cppnow wrapsfolly::available_concurrency(), which is confusingly named; consider renaming it (and any related identifiers) to reflect the new underlying API. - In
PrestoServer::initializeThreadPools, some thread counts directly callfolly::available_concurrency()while others use the cachedhwConcurrencyvalue; consider using a single cached value consistently (and renaming it) to avoid repeated calls and improve clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The helper `hardwareConcurrency()` in `Configs.cpp` now wraps `folly::available_concurrency()`, which is confusingly named; consider renaming it (and any related identifiers) to reflect the new underlying API.
- In `PrestoServer::initializeThreadPools`, some thread counts directly call `folly::available_concurrency()` while others use the cached `hwConcurrency` value; consider using a single cached value consistently (and renaming it) to avoid repeated calls and improve clarity.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Please add a release note following the Release Notes Guidelines to pass the failing but not required CI check. Please edit the PR title to follow semantic commit style to pass the failing and required CI check. See the failure in the test for advice. |
|
@steveburnett Thanks for looking. Should be done. |
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @yfeldblum
Posting this AI analysis for others to understand this change
-
Folly::hardware_concurrency()
This function provides the maximum theoretical parallelism supported by the CPU hardware. If you have a 20-core machine with hyperthreading enabled, this will likely return 40. However, if you run to restrict your program to a single core, this function still returns 40, which can lead to oversubscription if you spawn 40 threads. [1, 4] -
Folly::available_concurrency()
This function is designed to prevent performance degradation due to oversubscription in shared environments. It inspects the process's current CPU affinity and container settings.
Example: On a 20-core machine, if a cgroup restricts the process to 4 cores, will return 4, whereas will return 20.
Recommendation: For optimal performance, especially when using thread pools, available_concurrency is generally preferred as it adapts to the actual, restricted capacity of the environment. [1]
Summary: The name
hardware_concurrency, while parallel tostd::thread::hardware_concurrency, may be misleading. Migrate to the nameavailable_concurrency.Differential Revision: D96480646
Summary by Sourcery
Migrate Presto server concurrency configuration and related tests from folly::hardware_concurrency to folly::available_concurrency for determining core-based thread counts and limits.
Enhancements:
Tests: