impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Date Mon, 12 Sep 2016 23:59:50 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Chen Huang <paulhuang34@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message