Michael Ho has posted comments on this change.

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


Patch Set 3:

(1 comment)

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

Line 99:   bool BlockingPut(const T& val) {
> I think there's a pretty bad bug with how the signalling works. 
Yes, I have thought about it before. I have tested various locations for 
notification:

1. notify_one once an entry is consumed from get_list by BlockingGet()
2. notify_all when the get_list is empty
3. notify_one when the get_list is empty

Apparently, option (3) seems to give the best performance.

I have investigated option (2) and it appears that the thundering herd effect 
causes all scanner threads to start immediately leading to contention in memory 
allocation (e.g. memset takes 6x longer), causing some TPCH queries to regress.

I suspect the slow down in option (1) may have to do the extra signaling 
overhead per entry or it could be a side effect of the poor struct layout 
before so it may be worth retrying again.

In practice, a consumer can consume a row batch faster than a scanner thread 
can produce it so the effect of option (3) could be that it scatters out when 
the scanner threads get unblocked.


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