Michael Ho has posted comments on this change. Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue. ......................................................................
Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/4350/2/be/src/util/blocking-queue.h File be/src/util/blocking-queue.h: Line 40: /// held before 'write_lock_'. > describe the read_list_ and write_list_ swapping Done 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
