Michael Ho has posted comments on this change.

Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.

Patch Set 2:


File be/src/util/blocking-queue.h:

Line 40: /// held before 'write_lock_'.
> describe the read_list_ and write_list_ swapping

Line 58:   bool BlockingGet(T* out) {
> Threads in here will only detect shutdown after completely draining the rea
Actually, this seems to be the behavior of the old code too. Not sure if that's 
desirable though.

Line 76:           // Sleep with read lock held to block off other readers 
which cannot
> Won't this change the meaning of total_get_wait_time_ so that we're only co
That's true. For the case of scan-node, there is only one consumer so it's not 
a problem in practice but BlockingQueue is used somewhere else too so I will 
document it.

Line 111:     write_lock.unlock();
> Not your change but it'd be more consistent to use braced scoping to releas
I believe this is intentional as we want the lock to be dropped before calling 
notify_one() so the other thread woken up can grab the lock right away.

Line 147:       boost::lock_guard<boost::mutex> write_lock(write_lock_);
> Seems a little saner to grab both read_lock_ and write_lock_ although not s
Yes. In practice, the reader may sleep with read_lock held (intentionally) so 
it may result in unnecessary delay for the caller of this function. Will 
document the reasoning behind it.

Line 155:   uint32_t GetSize() const {
> It looks like this is sometimes called with lock held or not. Seems like we
It's always racy as we never hold read_lock when reading the size of read_list.

Line 161:   uint64_t total_get_wait_time() const {
> It doesn't look like total_get_wait_time and total_put_wait_time have any c
Yes, I thought about that but they may be useful for performance debugging. I 
just added two counters to hdfs-scan-node.h.

PS2, Line 185: mutex
> Unfortunately we don't have a condition variable implementation that works 
SpinLock should work but it may not be desirable. For instance, if the 
read_list is empty, the reader may sleep with read lock held, in which case 
SpinLock is a bad idea. Dropping and re-acquiring the read lock is possible but 
it may make BlockingGet() slightly more complicated and less performant.

Line 189:   boost::mutex write_lock_;
> We might be able to further reduce contention if we align the locks and oth
Good point. I rearranged the fields a bit and added some alignment hints.

To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Chen Huang <paulhuan...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to