-
Notifications
You must be signed in to change notification settings - Fork 415
Add a NestedNetlistRouter to enable integration with fine grained parallel router #2924
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
Conversation
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.
Some commenting and renaming suggestions.
Also add QoR data to the PR.
std::thread thread; | ||
std::queue<std::function<void()>> task_queue; | ||
std::mutex queue_mutex; | ||
std::condition_variable cv; |
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.
Shouldn't the name give an idea of what the condition variable controls/is used for, rather than just cv?
std::atomic<size_t> total_tasks_queued{0}; | ||
vtr::Timer pool_timer; // Track pool lifetime | ||
std::atomic<size_t> active_tasks{0}; | ||
std::mutex completion_mutex; |
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.
Comment these. (all the variables).
From our discussion: completion_mutex is to wait for all the tasks in the queue to finish.
std::condition_variable completion_cv; | ||
|
||
public: | ||
thread_pool(size_t thread_count) { |
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.
Between the class overview and here, you need to comment how this thread pool works.
dab0030
to
c6a5902
Compare
QoR data: custom_thread_pool_5.xlsx So |
Looks good. We can merge once the commenting and dead code feedback is addressed / resolved. |
01dbbfc
to
ac3fe5b
Compare
ac3fe5b
to
0c7800b
Compare
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.
Thanks for the additional commenting. I think you should add per function doxygen comments to the thread pool though (there are still some member functions with no comment).
TBB's thread pool doesn't let us block in tasks, so we have to implement a custom thread pool if we want to integrate a parallel netlist router with a parallel connection router.