-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Double buffer for cpp reader #8841
Double buffer for cpp reader #8841
Conversation
write_pos_(0), | ||
read_pos_(0) { | ||
std::thread prefetch( | ||
std::bind(&DoubleBufferReader::PrefetchThreadFunc, this)); |
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.
delete the bind is ok here.
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.
Amazing! Thanks.
if (read_pos_ >= kDoubleBufferSize) { | ||
read_pos_ = 0; | ||
} | ||
buffer_not_full_.notify_all(); |
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.
should be notify_one?
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.
For there is only one producer and one consumer here, notify_all()
and notify_one()
is same.
private: | ||
void PrefetchThreadFunc(); | ||
|
||
std::vector<std::vector<framework::LoDTensor>> buffer_; |
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.
Maybe a thread safe channel (queue) is better here.
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.
I'll take a look at that. Thanks.
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.
Looks Great. Only one comment, now we have two runtime type(the real type is decided in runtime -- framework::VarDesc::Raw, ReaderHolder. Could we figure out a better way to unify the runtime type?
|
||
prog = fluid.framework.Program() | ||
block = prog.current_block() | ||
startup_prog = fluid.framework.Program() |
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.
Why make the reader operator as a part of startup program?
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.
Because a reader should be created only once.
fixes #8847
Please review #8913 first.