Skip to content
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

Work queue group #35470

Merged
merged 11 commits into from
Sep 8, 2021
Merged

Conversation

liutiexing
Copy link
Contributor

PR types

New features

PR changes

Others

Describe

Add WorkQueueGroup type,multi Queue as a whole to WaitEmpty

@paddle-bot-old
Copy link

paddle-bot-old bot commented Sep 5, 2021

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@@ -157,7 +157,8 @@ InterpreterCore::InterpreterCore(const platform::Place& place,
garbages_.reset(new GarbageQueue());
max_memory_size_ = static_cast<size_t>(GetEagerDeletionThreshold());
cur_memory_size_ = 0;
gc_queue_ = CreateSingleThreadedWorkQueue();
WorkQueueOptions options;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options不需要赋值吗?我看默认线程数是0。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复

MultiThreadedWorkQueue(const MultiThreadedWorkQueue&) = delete;
void WorkQueueGroupImpl::AddTask(size_t queue_idx, std::function<void()> fn) {
assert(queue_idx < queues_.size());
if (queues_options_[queue_idx].track_task) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里是不是可以直接 if(nullptr == track_task_) ?

这里是支持 queues_options_ 部分track_task 为False, 部分为true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是的

MultiThreadedWorkQueue& operator=(const MultiThreadedWorkQueue&) = delete;
void WorkQueueGroupImpl::WaitQueueGroupEmpty() {
if (nullptr == tracker_) {
abort();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果tracker_为空,这里是不是给一些error信息,表示我们不支持在未指定track_task时,调用WaitQueueGroupEmpty接口,直接abort会让开发者很困惑?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

修改为PADDLE_THROW

virtual ~MultiThreadedWorkQueue() = default;
size_t WorkQueueGroupImpl::QueueNumThreads(size_t queue_idx) const {
assert(queue_idx < queues_.size());
return queues_[queue_idx]->NumThreads();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release版本不会执行执行assert,这里会存在越界风险,添加检查或者改为 queues_.at(queue_idx)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

struct WorkQueueOptions {
size_t num_threads{0};
bool allow_spinning{true};
bool track_task{false};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里是否提供一个有参构造函数比较好一些?track_task参数比较容易被误用。

”条款18:让接口容易被正确使用,不易被误用“ —— 《Effective C++ 》

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个options会扩充,提供了默认值。但是不提供构造函数,因为构造函数的参数中万一错位,很麻烦,必须显示使用一行代码修改不想要默认值的选项

Aurelius84
Aurelius84 previously approved these changes Sep 7, 2021
@wanghuancoder wanghuancoder merged commit a53460a into PaddlePaddle:develop Sep 8, 2021
2742195759 pushed a commit to 2742195759/Paddle that referenced this pull request Sep 10, 2021
* Split Tracker and WorkQueue

* add WorkQueueGroup

* add unittest

* fix

* update

* update

* fix compile
AnnaTrainingG pushed a commit to AnnaTrainingG/Paddle that referenced this pull request Sep 29, 2021
* Split Tracker and WorkQueue

* add WorkQueueGroup

* add unittest

* fix

* update

* update

* fix compile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants