impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Date Mon, 19 Sep 2016 17:10:49 GMT
Tim Armstrong has posted comments on this change.

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


Patch Set 4:

(8 comments)

I'm ok with this option if it generally provides better perf and avoids pathological behaviour.
Generally looks good, just comment and style comments.

http://gerrit.cloudera.org:8080/#/c/4350/4/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:

Line 36: /// if the queue is empty or full, respectively.
I think we should clarify when the unblocking happens. In BlockingGet() it is unblocked as
soon as an item is added, but in BlockingPut() it is not guaranteed (since we're not calling
put_cv_.NotifyOne() with the lock held).


Line 95:     put_cv_.NotifyOne();
Can you comment that this notification is racy: producers may not be notified if the queue
is partially empty.


Line 182:   /// Maximum number of elements in 'get_list_' + 'put_list_'.
clarify as "Maximum total number" - if a reader wasn't careful they might thing it meant the
maximum per list.


PS4, Line 190: > >
nit: we're generally using >> now that it works in c++11


http://gerrit.cloudera.org:8080/#/c/4350/4/be/src/util/condition-variable.h
File be/src/util/condition-variable.h:

Line 29: /// This has lower overhead than boost's implementation as it
Nit: lines are wrapped very short here


PS4, Line 31: CACHE_ALIGNED
Nit: here and in the other places: does it work to put this on the line before 'class'? That
seems a little more readable to me.


Line 43:   bool inline TimedWait(boost::unique_lock<boost::mutex>& lock,
What does the return value mean?


Line 50:   void inline NotifyOne() { pthread_cond_signal(&cv_); }
Nit: here and above, we usually put 'inline' before the return type.


-- 
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: 4
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