[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

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


IMPALA-4026: Implement double-buffering for BlockingQueue

With recent changes to improve the parquet scanner's efficency,
row batches are produced more quickly, leading to higher contention
in the blocking queue shared between scanner threads and the scan
node. The contention happens between different producers (i.e.
the scanner threads) and also to a lesser extent, between the
scanner threads and the scan node.

This change addresses the contention between the scanner threads
and the scan node by splitting the queue into a 'get_list_' and
a 'put_list_'. The consumers will consume from 'get_list_' until
it's exhausted while the producers will enqueue into 'put_list_'
until it's full. When 'get_list_' is exhausted, the consumer will
atomically swap the 'get_list_' with 'put_list_'. This reduces
the contention: 'get_list_' and 'put_list_' are protected by two
different locks so callers of BlockingGet() only contends for the
'put_lock_' when 'put_list_' is empty.

With this change, primitive_filter_bigint_non_selective improves
by 33.9%, going from 1.60s to 1.06s

Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Reviewed-on: http://gerrit.cloudera.org:8080/4350
Reviewed-by: Michael Ho 
Tested-by: Internal Jenkins
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/util/blocking-queue.h
A be/src/util/condition-variable.h
M be/src/util/thread-pool.h
6 files changed, 220 insertions(+), 66 deletions(-)

Approvals:
  Michael Ho: Looks good to me, approved
  Internal Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

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


Patch Set 12: Verified+1

-- 
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

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


Patch Set 12: Code-Review+2

Carry +2 forward.

-- 
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: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

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


Patch Set 11:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4350/11/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

PS11, Line 188: ADD_COUNTER
> nit: ADD_TIMER
Done


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

Line 190: // The size of 'get_list_' is read racily without holding 
'get_lock_'.
> .. to avoid taking the get_lock_ on the Put path.
Done


PS11, Line 208: callers
> remove the word "callers".  these functions wait on it directly.
Done


PS11, Line 228: callers
> remove "callers".
Done


-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Hello Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4350

to look at the new patch set (#12).

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

IMPALA-4026: Implement double-buffering for BlockingQueue

With recent changes to improve the parquet scanner's efficency,
row batches are produced more quickly, leading to higher contention
in the blocking queue shared between scanner threads and the scan
node. The contention happens between different producers (i.e.
the scanner threads) and also to a lesser extent, between the
scanner threads and the scan node.

This change addresses the contention between the scanner threads
and the scan node by splitting the queue into a 'get_list_' and
a 'put_list_'. The consumers will consume from 'get_list_' until
it's exhausted while the producers will enqueue into 'put_list_'
until it's full. When 'get_list_' is exhausted, the consumer will
atomically swap the 'get_list_' with 'put_list_'. This reduces
the contention: 'get_list_' and 'put_list_' are protected by two
different locks so callers of BlockingGet() only contends for the
'put_lock_' when 'put_list_' is empty.

With this change, primitive_filter_bigint_non_selective improves
by 33.9%, going from 1.60s to 1.06s

Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/util/blocking-queue.h
A be/src/util/condition-variable.h
M be/src/util/thread-pool.h
6 files changed, 220 insertions(+), 66 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/4350/12
-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

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


Patch Set 11: Code-Review+2

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4350/11/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

PS11, Line 188: ADD_COUNTER
nit: ADD_TIMER


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

Line 190: // The size of 'get_list_' is read racily without holding 
'get_lock_'.
.. to avoid taking the get_lock_ on the Put path.
(to explain the "why" not just the "what").


PS11, Line 208: callers
remove the word "callers".  these functions wait on it directly.


PS11, Line 228: callers
remove "callers".


-- 
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: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

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


Patch Set 7:

(2 comments)

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

PS7, Line 171: put_list_.size
> Right, I understand that, but that doesn't mean Size() should return any ra
Actually, even with holding the write lock, there is still a window in which 
the get_list_size_ may be stale so the caller of Size() may see zero even if 
get_list_ is non-empty. The new patch fixes this race by setting get_list_size_ 
too before dropping write lock.


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

PS9, Line 201: 
> That's my preference but i don't feel too strongly.
Tried removing const but it affects callers of Size() which also has const. 
Don't wanna open another can of worm.


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4350

to look at the new patch set (#11).

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

IMPALA-4026: Implement double-buffering for BlockingQueue

With recent changes to improve the parquet scanner's efficency,
row batches are produced more quickly, leading to higher contention
in the blocking queue shared between scanner threads and the scan
node. The contention happens between different producers (i.e.
the scanner threads) and also to a lesser extent, between the
scanner threads and the scan node.

This change addresses the contention between the scanner threads
and the scan node by splitting the queue into a 'get_list_' and
a 'put_list_'. The consumers will consume from 'get_list_' until
it's exhausted while the producers will enqueue into 'put_list_'
until it's full. When 'get_list_' is exhausted, the consumer will
atomically swap the 'get_list_' with 'put_list_'. This reduces
the contention: 'get_list_' and 'put_list_' are protected by two
different locks so callers of BlockingGet() only contends for the
'put_lock_' when 'put_list_' is empty.

With this change, primitive_filter_bigint_non_selective improves
by 33.9%, going from 1.60s to 1.06s

Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/util/blocking-queue.h
A be/src/util/condition-variable.h
M be/src/util/thread-pool.h
6 files changed, 222 insertions(+), 66 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/4350/11
-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

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


Patch Set 9:

(1 comment)

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

PS9, Line 201: mutable
> or just drop const altogether in these functions.
That's my preference but i don't feel too strongly.


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

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


Patch Set 9:

(1 comment)

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

PS9, Line 201: mutable
> Because of the const in SizeLocked(), total_get_wait_time() and total_put_w
or just drop const altogether in these functions.


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4350

to look at the new patch set (#10).

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

IMPALA-4026: Implement double-buffering for BlockingQueue

With recent changes to improve the parquet scanner's efficency,
row batches are produced more quickly, leading to higher contention
in the blocking queue shared between scanner threads and the scan
node. The contention happens between different producers (i.e.
the scanner threads) and also to a lesser extent, between the
scanner threads and the scan node.

This change addresses the contention between the scanner threads
and the scan node by splitting the queue into a 'get_list_' and
a 'put_list_'. The consumers will consume from 'get_list_' until
it's exhausted while the producers will enqueue into 'put_list_'
until it's full. When 'get_list_' is exhausted, the consumer will
atomically swap the 'get_list_' with 'put_list_'. This reduces
the contention: 'get_list_' and 'put_list_' are protected by two
different locks so callers of BlockingGet() only contends for the
'put_lock_' when 'put_list_' is empty.

With this change, primitive_filter_bigint_non_selective improves
by 33.9%, going from 1.60s to 1.06s

Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/util/blocking-queue.h
A be/src/util/condition-variable.h
M be/src/util/thread-pool.h
6 files changed, 221 insertions(+), 66 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/4350/10
-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Juan Yu 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

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


Patch Set 7:

(6 comments)

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

PS7, Line 55: put_cv_
should this be total_put_wait_time_ (i.e. the last "put" field)?  If we want to 
do this kind of check, it would be more robust to group put related fields in 
one struct, get stuff in another struct, then have this class contain a get and 
put field of these struct types, and write the assert that verifies those 
structs don't overlap cache lines.


PS7, Line 74: Calling
NotifyAll() is not used here to avoid ...


Line 74: // the queue size and when it calls Wait(). Calling 
NotifyAll() here may trigger
... when it calls Wait() in BlockingPut().


PS7, Line 100: HDFS scan node
is this explanation specific to HDFS scan node? We also use this queue in e.g. 
Kudu scan node and also ThreadPool.


PS7, Line 171: put_list_.size
why is this okay for the callers outside this class? we still have the split 
read problem.  also, the comment is a bit hand-wavy given that this could 
return a value that is never the true size of the queue. e.g. while swapping, 
might a caller see twice one of the deque sizes making it look like the queue 
is over capacity?  or, we could see a value too small, and that would violate 
the assumption being made in ImpalaServer::MembershipCallback() when it checks 
the work queue size.


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

PS7, Line 29:  
let's clarify: boost thread interruption


-- 
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: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

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


Patch Set 5:

(1 comment)

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

Line 102: put_cv_.NotifyOne();
> Not sure I understand the optimization you are referring to here.
I was getting the locks confused.


-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

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


Patch Set 5:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/4350/5/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

Line 79:   row_batches_put_timer_ = 
runtime_profile()->AddCounter("QueuePutTime", TUnit::TIME_NS);
> we usually do this in Open() or Prepare() (see other counters in e.g. HdfsS
Done


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

PS5, Line 75: NotitfyAll
> NotifyAll().
Done


PS5, Line 90: DCHECK
> nit: DCHECK_NE
DCHECK removed.


PS5, Line 98:  This may
: // imply that some writers may be sleeping on a partially 
empty queue
> Maybe "If this race occurs, a writer can stay blocked on a partially empty 
Done


PS5, Line 99: Given the major
: // use case is with HDFS scan node which has multiple 
producers and one consumer, it's
: // expected that some producers can make progress.
> Maybe more simply: "This should only occur when producers are faster than c
Done


Line 102: put_cv_.NotifyOne();
> is it worth explaining this race rather than fixing it?  Doesn't pthreads o
Not sure I understand the optimization you are referring to here.

The race here is that a thread can call put_cv_.NotifyOne() while another 
thread just checks the queue's size but before it calls put_cv_.Wait(). AFAIK, 
the only way to avoid this race is to also grab the "put_lock_" in 
BlockingGet() which kind of defeats the purpose of the change.


PS5, Line 137: GetSize
> this is an unfortunate name. I read it to be the size of the "Get" list.  M
Done


Line 171: // the queue's size could have changed once the lock is dropped.
> how do you know the deque::size() method doesn't need the synchronization (
Definitely expensive as writer can now block reader even if "get_list_" is not 
empty.

On x86, an aligned 64-bit read should be atomic. That said, it's a good point 
to that we cannot assume the implementation of dequeue::size(). Added an 
AtomicInt64 for the get_list's size to make sure all reads will be 32-bit 
consistent.


Line 197:   boost::scoped_ptr put_list_;
> why add this extra indirection?  couldn't we just do deque::swap() directly
Good point. Done. Also rearranged the class members a bit.


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

PS5, Line 29: doesn't have any logic to deal with thread interruption
> what's the implication of that?  are signals not handled properly?
The thread interruption feature has nothing to do with signal handling. It's a 
boost library feature which we don't use (at least for BlockingQueue). It's 
basically a way for one thread to interrupt another thread at well defined 
point in the code.

https://www.justsoftwaresolutions.co.uk/threading/thread-interruption-in-boost-thread-library.html


-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4350

to look at the new patch set (#7).

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

IMPALA-4026: Implement double-buffering for BlockingQueue

With recent changes to improve the parquet scanner's efficency,
row batches are produced more quickly, leading to higher contention
in the blocking queue shared between scanner threads and the scan
node. The contention happens between different producers (i.e.
the scanner threads) and also to a lesser extent, between the
scanner threads and the scan node.

This change addresses the contention between the scanner threads
and the scan node by splitting the queue into a 'get_list_' and
a 'put_list_'. The consumers will consume from 'get_list_' until
it's exhausted while the producers will enqueue into 'put_list_'
until it's full. When 'get_list_' is exhausted, the consumer will
atomically swap the 'get_list_' with 'put_list_'. This reduces
the contention: 'get_list_' and 'put_list_' are protected by two
different locks so callers of BlockingGet() only contends for the
'put_lock_' when 'put_list_' is empty.

With this change, primitive_filter_bigint_non_selective improves
by 37.5%, going from 1.6s to 1.0s

Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/util/blocking-queue.h
A be/src/util/condition-variable.h
M be/src/util/thread-pool.h
6 files changed, 213 insertions(+), 66 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/4350/7
-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

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


Patch Set 5:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/4350/5/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

Line 79:   row_batches_put_timer_ = 
runtime_profile()->AddCounter("QueuePutTime", TUnit::TIME_NS);
we usually do this in Open() or Prepare() (see other counters in e.g. 
HdfsScanNodeBase.  Also usually use ADD_COUNTER() macro (though I think we 
should get rid of those soon).


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

PS5, Line 90: DCHECK
nit: DCHECK_NE


Line 102: put_cv_.NotifyOne();
is it worth explaining this race rather than fixing it?  Doesn't pthreads 
optimize this case correctly (when holding the "wait lock" when doing a signal, 
it should just transfer the destination thread to the lock's wait queue rather 
than waking it up).  But maybe we aren't sure that optimization is implemented?


PS5, Line 137: GetSize
this is an unfortunate name. I read it to be the size of the "Get" list.  Maybe 
we could rename it Size()?


Line 171: // the queue's size could have changed once the lock is dropped.
how do you know the deque::size() method doesn't need the synchronization (on 
x86 it's probably okay but maybe not other platforms). Would taking both locks 
here be prohibitively expensive?


Line 197:   boost::scoped_ptr put_list_;
why add this extra indirection?  couldn't we just do deque::swap() directly on 
the deque, rather than using the ptr swap?


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

PS5, Line 29: doesn't have any logic to deal with thread interruption
what's the implication of that?  are signals not handled properly?


-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Michael Ho has uploaded a new patch set (#5).

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

IMPALA-4026: Implement double-buffering for BlockingQueue

With recent changes to improve the parquet scanner's efficency,
row batches are produced more quickly, leading to higher contention
in the blocking queue shared between scanner threads and the scan
node. The contention happens between different producers (i.e.
the scanner threads) and also to a lesser extent, between the
scanner threads and the scan node.

This change addresses the contention between the scanner threads
and the scan node by splitting the queue into a 'get_list_' and
a 'put_list_'. The consumers will consume from 'get_list_' until
it's exhausted while the producers will enqueue into 'put_list_'
until it's full. When 'get_list_' is exhausted, the consumer will
atomically swap the 'get_list_' with 'put_list_'. This reduces
the contention: 'get_list_' and 'put_list_' are protected by two
different locks so callers of BlockingGet() only contends for the
'put_lock_' when 'put_list_' is empty.

Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/util/blocking-queue.h
A be/src/util/condition-variable.h
5 files changed, 198 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/4350/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

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


Patch Set 4:

(8 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 
The new comment in BlockingGet() should help clarify it.


Line 95: put_cv_.NotifyOne();
> Can you comment that this notification is racy: producers may not be notifi
Done


Line 182:   /// Maximum number of elements in 'get_list_' + 'put_list_'.
> clarify as "Maximum total number" - if a reader wasn't careful they might t
Done


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


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
Done


PS4, Line 31: CACHE_ALIGNED
> Nit: here and in the other places: does it work to put this on the line bef
Moved to the end of the class definition. That should be less confusing.


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


Line 50:   void inline NotifyOne() { pthread_cond_signal(_); }
> Nit: here and above, we usually put 'inline' before the return type.
Fixed. This convention is a bit inconsistent with where we put 
ALWAYS_INLINE/IR_NO_INLINE/IR_ALWAYS_INLINE.


-- 
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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

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) {
> We discussed this a little offline and I feel like I understand why option 
The new patch reverted to using option(1). This doesn't provide as much 
performance in the non-selective scan but this may avoid some pathological case 
in thread scheduling.


-- 
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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue

[Michael Ho (Code Review)]

Michael Ho has uploaded a new patch set (#4).

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

IMPALA-4026: Implement double-buffering for BlockingQueue

With recent changes to improve the parquet scanner's efficency,
row batches are produced more quickly, leading to higher contention
in the blocking queue shared between scanner threads and the scan
node. The contention happens between different producers (i.e.
the scanner threads) and also to a lesser extent, between the
scanner threads and the scan node.

This change addresses the contention between the scanner threads
and the scan node by splitting the queue into a 'read_list_' and
a 'write_list_'. The consumers will consume from 'read_list_' until
it's exhausted while the producers will enqueue into 'write_list_'
until it's full. When 'read_list_' is exhausted, the consumer will
atomically swap the 'read_list_' with 'write_list_'. This reduces
the contention/overhead: 'read_list_' and 'write_list_' are protected
by two different locks so consumer only contends for the
write lock when 'read_list_' is empty.

Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/util/blocking-queue.h
A be/src/util/condition-variable.h
5 files changed, 184 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/4350/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.

[Tim Armstrong (Code Review)]

Tim Armstrong 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) {
> Yes, I have thought about it before. I have tested various locations for no
We discussed this a little offline and I feel like I understand why option (3) 
works in practice now - when N producers are outpacing consumers, this switches 
to a mode where the steady state is a nearly empty queue with k producers 
sitting blocked and N - k producers working. Essentially there are some 
additional elements in the queue because the blocked producers also have some 
row batches ready to add to the queue immediately.

I'm ok moving forward with it so long as we document the expected behaviour. 
Perhaps it's adequate to document the expect behaviour with a faster consumer, 
matched producer/consumer and fast producers.

Have you benchmarked this with concurrent queries or workloads with many 
concurrent scans? One concern with the empty queue is that if the queue is 
empty and a producer isn't scheduled immediately, the consumer may end up 
waiting on the condition variable for every batch and potentially getting 
swapped it. We could perhaps work around that if the producer added its item to 
the queue *before* blocking, so that the the consumer can get the item without 
requiring a context switch.


-- 
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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.

[Michael Ho (Code Review)]

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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

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


Patch Set 3:

(6 comments)

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

PS3, Line 188: put_list
> shouldn't these members have _ at the end?
I just converted this to a struct :-).


Line 197:   boost::mutex get_lock;
> Shouldn't we align get_lock?
It's implicitly aligned after I added the attribute to ConditionVariable as the 
compiler seems to pad that struct to 64-byte. I will add to DCHECK to verify 
that's the case.


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

Line 28: /// Simple wrapper around POSIX pthread condition variable.
> Explain briefly how it differs from the boost cv?
Sure.


PS3, Line 29: __attribute__ ((aligned(64)))
> What do you think about putting this in compiler-util.h as CACHE_ALIGN or s
Good idea.


PS3, Line 29: struct
> class?
Can you actually put __attribute__ ((aligned(64))) on class in C++ ?


PS3, Line 61: cv
> cv_
I think struct requires no "_" ?!


-- 
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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.

[Michael Ho (Code Review)]

Michael Ho has uploaded a new patch set (#3).

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

IMPALA-4026: Implement double-buffering for BlockingQueue.

With recent changes to improve the parquet scanner's efficency,
row batches are produced more quickly, leading to higher contention
in the blocking queue shared between scanner threads and the scan
node. The contention happens between different producers (i.e.
the scanner threads) and also to a lesser extent, between the
scanner threads and the scan node.

This change addresses the contention between the scanner threads
and the scan node by splitting the queue into a 'read_list_' and
a 'write_list_'. The consumers will consume from 'read_list_' until
it's exhausted while the producers will enqueue into 'write_list_'
until it's full. When 'read_list_' is exhausted, the consumer will
atomically swap the 'read_list_' with 'write_list_'. This reduces
the contention/overhead in two ways: (1) 'read_list_' and 'write_list_'
are protected by two different locks so consumer only contends for the
write lock when 'read_list_' is empty. (2) the consumer only signals
the producer after an entire 'read_list_' is consumed instead of
signalling it per entry in the 'read_list_'.

With this change, the regression in primitive_filter_bigint_non_selective
went from 1.6s to 0.8s, improving by 50%.

Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
---
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/util/blocking-queue.h
A be/src/util/condition-variable.h
4 files changed, 196 insertions(+), 70 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/4350/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Chen Huang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Tim Armstrong