-
Notifications
You must be signed in to change notification settings - Fork 659
New ThreadPool + thread pool facade #6224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
cf9f226
443659b
3d9e221
f877ef3
81cd10a
3dab38e
b9d5f8d
98a7e65
d1c7962
1eca945
a9ec7fa
8709e96
dc92497
783d100
d09c10e
7dd4d8d
5d9cf4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||||||||||||||
| // Copyright (c) 2024-2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||||||||||||||||||||||
| // Copyright (c) 2024-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||||||||||||||||||||||
| // | ||||||||||||||||||||||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||
| // you may not use this file except in compliance with the License. | ||||||||||||||||||||||
|
|
@@ -24,6 +24,7 @@ | |||||||||||||||||||||
| #include "dali/pipeline/executor/executor2/exec_graph.h" | ||||||||||||||||||||||
| #include "dali/pipeline/executor/executor2/stream_assignment.h" | ||||||||||||||||||||||
| #include "dali/pipeline/operator/builtin/input_operator.h" | ||||||||||||||||||||||
| #include "dali/pipeline/util/new_thread_pool.h" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| namespace dali { | ||||||||||||||||||||||
| namespace exec2 { | ||||||||||||||||||||||
|
|
@@ -42,9 +43,9 @@ void LimitBackendConcurrency(ExecGraph &graph, OpType backend, int max_concurren | |||||||||||||||||||||
| void ApplyConcurrencyLimit(ExecGraph &graph, OperatorConcurrency concurrency) { | ||||||||||||||||||||||
| switch (concurrency) { | ||||||||||||||||||||||
| case OperatorConcurrency::Full: | ||||||||||||||||||||||
| // TODO(michalz): Fix ThreadPool. | ||||||||||||||||||||||
| LimitBackendConcurrency(graph, OpType::CPU); | ||||||||||||||||||||||
| break; // other operators have no restrictions | ||||||||||||||||||||||
| if (!UseNewThreadPool()) // old thread pool is not thread safe | ||||||||||||||||||||||
| LimitBackendConcurrency(graph, OpType::CPU); | ||||||||||||||||||||||
| break; | ||||||||||||||||||||||
| case OperatorConcurrency::Backend: | ||||||||||||||||||||||
| LimitBackendConcurrency(graph, OpType::CPU); | ||||||||||||||||||||||
| LimitBackendConcurrency(graph, OpType::GPU); | ||||||||||||||||||||||
|
|
@@ -345,18 +346,37 @@ class Executor2::Impl { | |||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| void SetupThreadPool() { | ||||||||||||||||||||||
| if (graph_info_.num_cpu > 0) { | ||||||||||||||||||||||
| tp_ = std::make_unique<ThreadPool>( | ||||||||||||||||||||||
| config_.thread_pool_threads, | ||||||||||||||||||||||
| config_.device.value_or(CPU_ONLY_DEVICE_ID), | ||||||||||||||||||||||
| config_.set_affinity, | ||||||||||||||||||||||
| "Executorv_v2"); | ||||||||||||||||||||||
| thread_pool_wrappers_.clear(); | ||||||||||||||||||||||
| new_tp_.reset(); | ||||||||||||||||||||||
| old_tp_.reset(); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (UseNewThreadPool()) { | ||||||||||||||||||||||
| std::cerr << "\n!!! Forced use of NewThreadPool !!!" << std::endl; | ||||||||||||||||||||||
| if (graph_info_.num_cpu > 0) { | ||||||||||||||||||||||
|
Comment on lines
+353
to
+355
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Debug This diagnostic print was presumably added during development but should not be shipped:
Suggested change
Remove the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @greptileai This will be removed before merging, but is necessary to validate that the flag is properly set and used in CI.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it! That makes sense for CI validation. Consider adding a TODO comment on that line so it's clear it needs to be removed:
Suggested change
This way it won't accidentally slip through code review. |
||||||||||||||||||||||
| new_tp_ = std::make_unique<NewThreadPool>( | ||||||||||||||||||||||
| config_.thread_pool_threads, | ||||||||||||||||||||||
| config_.device.value_or(CPU_ONLY_DEVICE_ID), | ||||||||||||||||||||||
| config_.set_affinity, | ||||||||||||||||||||||
| "Executor_v2"); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| for (auto &n : graph_.Nodes()) { | ||||||||||||||||||||||
| if (n.backend == OpType::CPU) { | ||||||||||||||||||||||
| thread_pool_wrappers_.push_back(std::make_unique<ThreadPoolFacade>(new_tp_.get())); | ||||||||||||||||||||||
| n.env.thread_pool = thread_pool_wrappers_.back().get(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| tp_.reset(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| for (auto &n : graph_.Nodes()) { | ||||||||||||||||||||||
| if (n.backend == OpType::CPU) | ||||||||||||||||||||||
| n.env.thread_pool = tp_.get(); | ||||||||||||||||||||||
| if (graph_info_.num_cpu > 0) { | ||||||||||||||||||||||
| old_tp_ = std::make_unique<OldThreadPool>( | ||||||||||||||||||||||
| config_.thread_pool_threads, | ||||||||||||||||||||||
| config_.device.value_or(CPU_ONLY_DEVICE_ID), | ||||||||||||||||||||||
| config_.set_affinity, | ||||||||||||||||||||||
| "Executor_v2"); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| for (auto &n : graph_.Nodes()) { | ||||||||||||||||||||||
| if (n.backend == OpType::CPU) | ||||||||||||||||||||||
| n.env.thread_pool = old_tp_.get(); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -421,7 +441,9 @@ class Executor2::Impl { | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Runtime environment | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| std::unique_ptr<ThreadPool> tp_; | ||||||||||||||||||||||
| std::unique_ptr<OldThreadPool> old_tp_; | ||||||||||||||||||||||
| std::unique_ptr<NewThreadPool> new_tp_; | ||||||||||||||||||||||
| std::vector<std::unique_ptr<ThreadPool>> thread_pool_wrappers_; | ||||||||||||||||||||||
| std::queue<tasking::TaskFuture> pending_outputs_; | ||||||||||||||||||||||
| std::vector<CUDAStreamLease> streams_; | ||||||||||||||||||||||
| std::map<std::string, ExecNode *, std::less<>> node_map_; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify why concurrency policy and thread pool setup calls both use
UseNewThreadPool()Both
ApplyConcurrencyLimit(line 117) andSetupThreadPool()(line 119) callUseNewThreadPool(), relying on the fact that it evaluates itsstatic boolonly once. This subtle coupling is worth documenting: