blocking_queue #2366
blocking_queue #2366
Conversation
| namespace caffe { | ||
|
|
||
| template<typename T> | ||
| class blocking_queue { |
There was a problem hiding this comment.
should be called BlockingQueue (https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Type_Names)
|
I glanced over this and it LGTM other than my capitalization nitpick, but someone who's used |
| #include <boost/thread.hpp> | ||
| #include <string> | ||
|
|
||
| #include "caffe/util/blocking_queue.hpp" |
There was a problem hiding this comment.
This should be the first #include.
There was a problem hiding this comment.
Lint wants the system imports first
There was a problem hiding this comment.
Yeah, I think lint is wrong here with respect to the Google style guide; ideally we'll fix that at some point, but we've been following lint on this point in other places so best just to stay consistent here.
|
LGTM once you add a proper commit message :) |
|
Good point, fixed |
| void BlockingQueue<T>::push(const T& t) { | ||
| boost::mutex::scoped_lock lock(sync_->mutex_); | ||
| queue_.push(t); | ||
| lock.unlock(); |
There was a problem hiding this comment.
Does the push method need to make an explicit unlock() call? Have you performed any timing experiments that suggest that unlocking helps in reducing lock contention?
There was a problem hiding this comment.
Calling notify_one is best done outside the locked section. It can be done inside, but this way allows the notified thread to run immediately.
There was a problem hiding this comment.
Some reference:
http://en.cppreference.com/w/cpp/thread/condition_variable/notify_one
The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock. However, some implementations (in particular many implementations of pthreads) recognize this situation and avoid this "hurry up and wait" scenario by transferring the waiting thread from the condition variable's queue directly to the queue of the mutex within the notify call, without waking it up.
Part of #2351, I could not switch to #2167 because I needed boost::thread forward declared. Also instead of having a max size, I prefer to use queues in pairs: free and full, so that items can be reused when the consumer is done with them. It is particularly useful for GPU memory which is costly to allocate and destroy.