You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2016/09/09 18:33:24 UTC
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4350
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 scanner threads and scan
node.
This change addresses the contention between scanner threads
and 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_'. This change also converts
BlockingQueue to using POSIX pthread primitives instead of boost
library which introduces some unncessary overhead (as observed
from VTune profiles).
With this change, the regression in primitive_filter_bigint_non_selective
went from 1.6s to 0.8s, improving by 50%.
+---------------------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+---------------------+-----------------------+---------+------------+------------+----------------+
| TARGETED-PERF(_300) | parquet / none / none | 34.74 | -4.56% | 9.75 | -4.50% |
+---------------------+-----------------------+---------+------------+------------+----------------+
+---------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Num Clients | Iters |
+---------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+
| TARGETED-PERF(_300) | primitive_conjunct_ordering_1 | parquet / none / none | 10.72 | 9.84 | +8.95% | 2.57% | 0.53% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_string_selective | parquet / none / none | 1.09 | 1.01 | +7.42% | 2.59% | 4.91% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_broadcast_join_2 | parquet / none / none | 6.01 | 5.71 | +5.38% | 0.07% | 0.96% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_string_non_selective | parquet / none / none | 1.04 | 0.99 | +4.89% | 2.44% | 2.46% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_conjunct_ordering_5 | parquet / none / none | 40.60 | 38.93 | +4.29% | 3.50% | 1.02% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_decimal_selective | parquet / none / none | 0.92 | 0.88 | +3.70% | 0.06% | 2.69% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_broadcast_join_1 | parquet / none / none | 1.70 | 1.66 | +2.74% | 1.26% | 1.43% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_empty_build_join_1 | parquet / none / none | 13.31 | 13.07 | +1.79% | 1.04% | 0.98% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_bigint_lowndv | parquet / none / none | 3.62 | 3.59 | +0.73% | 2.10% | 0.04% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_string_like | parquet / none / none | 5.77 | 5.74 | +0.45% | 3.07% | 1.76% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_broadcast_join_3 | parquet / none / none | 58.96 | 58.73 | +0.39% | 0.43% | 0.27% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_bigint_highndv | parquet / none / none | 23.84 | 23.78 | +0.26% | 0.67% | 1.27% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_bigint_pk | parquet / none / none | 89.36 | 89.41 | -0.05% | 2.64% | 1.74% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_conjunct_ordering_3 | parquet / none / none | 1.53 | 1.53 | -0.23% | 0.37% | 0.07% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_conjunct_ordering_4 | parquet / none / none | 1.17 | 1.18 | -1.22% | 1.30% | 0.01% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_decimal_lowndv.test | parquet / none / none | 3.49 | 3.62 | -3.48% | 1.45% | 0.72% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_bigint_selective | parquet / none / none | 0.64 | 0.67 | -3.66% | 4.04% | 0.09% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_exchange_shuffle | parquet / none / none | 76.82 | 80.02 | -4.00% | 0.28% | 1.07% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_exchange_broadcast | parquet / none / none | 84.30 | 88.32 | -4.56% | 0.67% | 1.02% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_top-n_all | parquet / none / none | 34.24 | 35.89 | -4.59% | 1.85% | 0.26% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_topn_bigint | parquet / none / none | 5.02 | 5.28 | -4.89% | 1.52% | 0.98% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_orderby_bigint | parquet / none / none | 30.72 | 32.55 | -5.62% | 0.00% | 0.94% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_conjunct_ordering_2 | parquet / none / none | 73.94 | 78.98 | -6.39% | 3.50% | 2.23% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_shuffle_join_one_to_many_string_with_groupby | parquet / none / none | 228.99 | 244.67 | -6.41% | 0.29% | 0.74% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_orderby_all | parquet / none / none | 108.38 | 116.47 | -6.94% | 0.73% | 2.14% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_decimal_highndv | parquet / none / none | 24.78 | 26.76 | -7.40% | 0.08% | 4.73% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_shuffle_join_union_all_with_groupby | parquet / none / none | 74.98 | 83.63 | -10.34% | 3.71% | 1.21% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_decimal_non_selective | parquet / none / none | 0.88 | 1.12 | I -21.79% | 1.14% | 0.08% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_bigint_non_selective | parquet / none / none | 0.74 | 1.60 | I -53.54% | 3.50% | 1.53% | 1 | 3 |
+---------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+
Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
---
M be/src/util/blocking-queue.h
1 file changed, 113 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/4350/1
--
To view, visit http://gerrit.cloudera.org:8080/4350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4350/1//COMMIT_MSG
Commit Message:
Line 29: BlockingQueue to using POSIX pthread primitives instead of boost
> In one of the backtrace from VTune, there was about 25% of the time of pthr
I meant to say boost::condition_variable::wait() in the previous comment, not pthread_cond_wait().
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 (#9).
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/9
--
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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<std::deque<T>> 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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4350/1//COMMIT_MSG
Commit Message:
Line 29: BlockingQueue to using POSIX pthread primitives instead of boost
> I meant to say boost::condition_variable::wait() in the previous comment, n
Btw, this overhead seems to be related to the thread interruption support in boost library (https://www.justsoftwaresolutions.co.uk/threading/thread-interruption-in-boost-thread-library.html). Given the way we shut down the blocking queue, I am not sure if we need this. May be we can compile it out of the boost library.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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<std::deque<T>> 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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 1:
(5 comments)
The new algorithm makes sense to me. We could probably get fancier but this seems like a nice balance of simplicity and performance.
Mostly high-level comments so far
http://gerrit.cloudera.org:8080/#/c/4350/1//COMMIT_MSG
Commit Message:
Line 29: BlockingQueue to using POSIX pthread primitives instead of boost
This seems strange. I looked at ./boost_1_57_0/boost/thread/pthread/mutex.hpp in the toolchain and the boost mutex is just a thin wrapper around pthread_mutex.
Line 45: | TARGETED-PERF(_300) | primitive_conjunct_ordering_1 | parquet / none / none | 10.72 | 9.84 | +8.95% | 2.57% | 0.53% | 1 | 3 |
Any idea why these regressed? Might be good to understand if there's something we can do to fix the regression or improve performance further.
http://gerrit.cloudera.org:8080/#/c/4350/1/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
Line 65: pthread_mutex_lock(&read_lock_);
I think it would be good to stick with the unique_lock interface - I find it harder to convince myself that everything is unlocked on all code paths otherwise.
I'm not sure why the boost mutex is slower, but maybe it's because it's checking the error codes and throwing an exception.
If this is the problem, I think we should consider writing our own wrapper for pthread_mutex without the exceptions (maybe just DCHECKs?). The required lockable interface is pretty simple - SpinLock already implements it.
PS1, Line 186: shutted down.
shut down.
Line 201: /// Guards against concurrent access to 'write_list_'.
Maybe document lock order here too?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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/condition-variable.h
File be/src/util/condition-variable.h:
PS3, Line 29: struct
> Can you actually put __attribute__ ((aligned(64))) on class in C++ ?
Actually, this seems to compile with class too. Not sure why I had the preconception that it doesn't work. Updated in the next patch.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 1:
Can you show the difference between the nightly run and this patch then if that's the true baseline? It looks like some queries regressed relative to that too, e.g. primitive_conjunct_ordering_5
You can generate a benchmark report relative to a different baseline by passing the the two .json files into tests/benchmark/report_benchmark_results.py
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4350/2/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
PS2, Line 185: mutex
> worth changing these to SpinLock, which I believe are a bit faster to lock/
Unfortunately we don't have a condition variable implementation that works with SpinLock (that would be very nice to have).
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Juan Yu (Code Review)" <ge...@cloudera.org>.
Juan Yu 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/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:
PS9, Line 188: Queue
Could you specify which queue this is? will help people understand what's the metric for.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2).
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/util/blocking-queue.h
1 file changed, 89 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/4350/2
--
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue
......................................................................
Patch Set 5: Code-Review+1
(3 comments)
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().
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 queue until the next BlockingGet() call." And I think it would be clearer if the consequence of the race was explained before why it was benign.
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 consumers, so is unlikely to affect overall throughput".
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 2:
(5 comments)
Thanks for switching back to unique_lock, it makes it a bit easier to reason about for me. Change looks good, just a few comments.
http://gerrit.cloudera.org:8080/#/c/4350/2/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
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 counting the single thread that got the lock?
This is ok since I don't think the counter is contractual, and the new definition may be more useful, but should update the comment.
Line 111: write_lock.unlock();
Not your change but it'd be more consistent to use braced scoping to release the lock instead of this explicit unlock.
Line 155: uint32_t GetSize() const {
It looks like this is sometimes called with lock held or not. Seems like we should document this (i.e. caller should hold lock for non-racy read).
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 callers - could just get rid ofthe functions?
Line 189: boost::mutex write_lock_;
We might be able to further reduce contention if we align the locks and other data structures so that they're guaranteed to not share 64-bit cache lines. Maybe group read and write members together so that the can share cache lines, then align the first member of both groups to 64 bytes?
They're only 40 bytes on my system, so with the current layout read_lock and write_lock may be on the same cache line:
#include <pthread.h>
#include <stdio.h>
#include <stddef.h>
#include <boost/thread/mutex.hpp>
struct test {
boost::mutex mutex1;
boost::mutex mutex2;
};
int main() {
printf("sizeof(pthread_mutex_t)=%ld\n", sizeof(pthread_mutex_t));
printf("sizeof(boost::mutex)=%ld\n", sizeof(boost::mutex));
printf("sizeof(pthread_cond_t)=%ld\n", sizeof(pthread_cond_t));
printf("offsetof(...)=%ld\n", offsetof(struct test, mutex2));
return 0;
}
[~]$ g++ mutex.cc -lboost_system -o mutex&& ./mutex
sizeof(pthread_mutex_t)=40
sizeof(boost::mutex)=40
sizeof(pthread_cond_t)=48
offsetof(...)=40
[~]$
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4350/1//COMMIT_MSG
Commit Message:
Line 29: BlockingQueue to using POSIX pthread primitives instead of boost
> This seems strange. I looked at ./boost_1_57_0/boost/thread/pthread/mutex.h
In one of the backtrace from VTune, there was about 25% of the time of pthread_cond_wait() spent in the destructor of interruption_checker().
Similarly, there is an extra mutex_lock() in notify_one() and notify_all(). It's unclear what these extra checks in boost library are buying us.
Line 45: | TARGETED-PERF(_300) | primitive_conjunct_ordering_1 | parquet / none / none | 10.72 | 9.84 | +8.95% | 2.57% | 0.53% | 1 | 3 |
> Any idea why these regressed? Might be good to understand if there's someth
Not sure if this is related to this patch. The nightly run from last night also shows similar (and in fact larger) level of regression:
http://sandbox.jenkins.cloudera.com/job/impala-workload-runner-16node-cdh5/1763/
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 2:
(3 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
Line 58: bool BlockingGet(T* out) {
Threads in here will only detect shutdown after completely draining the read_list_. That seems like a slight behavioral change, not sure if it can cause problems. Consider grabbing the read_lock_ Shutdown() and checking for shutdown here regardless of read_list_ size.
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 strictly necessary.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4350/9/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
PS9, Line 188: d(const boo
> that's not true -- the atomic means it's not a "racy read". I think what y
It's racy in the sense that it's not holding 'get_lock_'.
PS9, Line 201: /// Ple
> why do we need these mutable? it wouldn't make sense to declare a 'const B
Because of the const in SizeLocked(), total_get_wait_time() and total_put_wait_time().
--
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Chen Huang (Code Review)" <ge...@cloudera.org>.
Chen Huang has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4350/1/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
PS1, Line 192: /// 'get' callers wait on this.
: mutable pthread_cond_t get_cv_;
:
: /// 'put' callers wait on this.
: mutable pthread_cond_t put_cv_;
:
: /// Guards against concurrent access to 'read_list_'.
: mutable pthread_mutex_t read_lock_;
:
: /// Guards against concurrent access to 'write_list_'.
: mutable pthread_mutex_t write_lock_;
why do we add 'mutable' here? they are used in non-const member functions anyway
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue
......................................................................
Patch Set 9:
(3 comments)
Is there a new patchset that addresses Juan's change?
http://gerrit.cloudera.org:8080/#/c/4350/7/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
PS7, Line 171:
> Yes, this can be split into SizeLocked() which requires "put_lock_" be held
Right, I understand that, but that doesn't mean Size() should return any random value. The assumption that mpalaServer::MembershipCallback(), for example, seems valid. Holding the write lock does solve this though since it protects against the swap which is when the double counting (or zero counting) can occur.
http://gerrit.cloudera.org:8080/#/c/4350/9/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
PS9, Line 188: read racily
that's not true -- the atomic means it's not a "racy read". I think what you mean is the get and put sizes are not read atomically together?
PS9, Line 201: mutable
why do we need these mutable? it wouldn't make sense to declare a 'const BlockingQueue' (and it wouldn't work since a lot of these other fields would need to be mutable as well).
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 1:
(3 comments)
It appears to me that primitive_conjunct_odering_5 and primitive_conjunct_ordering_1 have quite a bit of variance. I compared the result at the following two baseline runs:
http://sandbox.jenkins.cloudera.com/job/impala-private-perf-16node/234/
at commit 1a83f8bbe871df0527923e6f131a295270e54d33
http://sandbox.jenkins.cloudera.com/job/impala-private-perf-16node/233/
at commit 4fd25114d2ca23205396af1c16dcde3d3bec69fb
The result for conjunt_ordering_5 ranges from 27.16 to 44.45 while the reference baseline is 38.93. Similarly, conjunct_ordering_1 ranges from 10.95 to 11.08 while the reference baseline is 9.84. My baseline should be based on run 233:
+---------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Num Clients | Iters |
+---------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+
| TARGETED-PERF(_300) | primitive_filter_string_selective | parquet / none / none | 1.09 | 0.99 | +10.31% | 2.59% | 2.49% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_conjunct_ordering_2 | parquet / none / none | 73.94 | 70.22 | +5.29% | 3.50% | 0.29% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_broadcast_join_2 | parquet / none / none | 6.01 | 5.73 | +5.04% | 0.07% | 0.44% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_broadcast_join_1 | parquet / none / none | 1.70 | 1.62 | +4.83% | 1.26% | 3.04% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_exchange_broadcast | parquet / none / none | 84.30 | 81.13 | +3.90% | 0.67% | 0.35% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_string_non_selective | parquet / none / none | 1.04 | 1.01 | +2.31% | 2.44% | 0.21% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_bigint_lowndv | parquet / none / none | 3.62 | 3.57 | +1.44% | 2.10% | 0.71% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_bigint_pk | parquet / none / none | 89.36 | 88.84 | +0.59% | 2.64% | 1.25% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_broadcast_join_3 | parquet / none / none | 58.96 | 58.61 | +0.59% | 0.43% | 0.08% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_orderby_bigint | parquet / none / none | 30.72 | 30.64 | +0.26% | 0.00% | 0.00% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_decimal_selective | parquet / none / none | 0.92 | 0.92 | +0.14% | 0.06% | 0.53% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_bigint_highndv | parquet / none / none | 23.84 | 23.82 | +0.08% | 0.67% | 0.31% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_empty_build_join_1 | parquet / none / none | 13.31 | 13.32 | -0.10% | 1.04% | 0.54% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_string_like | parquet / none / none | 5.77 | 5.84 | -1.28% | 3.07% | 2.55% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_conjunct_ordering_4 | parquet / none / none | 1.17 | 1.18 | -1.33% | 1.30% | 0.01% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_shuffle_join_one_to_many_string_with_groupby | parquet / none / none | 228.99 | 232.12 | -1.35% | 0.29% | 1.01% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_conjunct_ordering_3 | parquet / none / none | 1.53 | 1.55 | -1.87% | 0.37% | 4.97% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_orderby_all | parquet / none / none | 108.38 | 110.56 | -1.97% | 0.73% | 1.21% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_conjunct_ordering_1 | parquet / none / none | 10.72 | 10.95 | -2.10% | 2.57% | 1.82% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_decimal_lowndv.test | parquet / none / none | 3.49 | 3.62 | -3.44% | 1.45% | 0.63% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_bigint_selective | parquet / none / none | 0.64 | 0.67 | -3.64% | 4.04% | 0.16% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_exchange_shuffle | parquet / none / none | 76.82 | 80.29 | -4.32% | 0.28% | 0.41% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_shuffle_join_union_all_with_groupby | parquet / none / none | 74.98 | 79.89 | -6.14% | 3.71% | 0.82% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_top-n_all | parquet / none / none | 34.24 | 36.63 | -6.52% | 1.85% | 0.78% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_decimal_non_selective | parquet / none / none | 0.88 | 0.94 | -7.04% | 1.14% | 2.49% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_groupby_decimal_highndv | parquet / none / none | 24.78 | 26.66 | -7.07% | 0.08% | 0.22% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_topn_bigint | parquet / none / none | 5.02 | 5.43 | -7.42% | 1.52% | 0.20% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_conjunct_ordering_5 | parquet / none / none | 40.60 | 44.45 | -8.64% | 3.50% | 3.56% | 1 | 3 |
| TARGETED-PERF(_300) | primitive_filter_bigint_non_selective | parquet / none / none | 0.74 | 1.62 | I -54.22% | 3.50% | 0.01% | 1 | 3 |
+---------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+
http://gerrit.cloudera.org:8080/#/c/4350/1//COMMIT_MSG
Commit Message:
Line 29: BlockingQueue to using POSIX pthread primitives instead of boost
> Maybe we should just implement our own wrapper class around pthread's condi
That sounds like a reasonable middle ground. Will look into it.
http://gerrit.cloudera.org:8080/#/c/4350/1/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
Line 201: /// Guards against concurrent access to 'write_list_'.
> Maybe document lock order here too?
Done
PS1, Line 192: /// 'get' callers wait on this.
: mutable pthread_cond_t get_cv_;
:
: /// 'put' callers wait on this.
: mutable pthread_cond_t put_cv_;
:
: /// Guards against concurrent access to 'read_list_'.
: mutable pthread_mutex_t read_lock_;
:
: /// Guards against concurrent access to 'write_list_'.
: mutable pthread_mutex_t write_lock_;
> why do we add 'mutable' here? they are used in non-const member functions a
Yes, they are probably not needed.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4350/2/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
PS2, Line 185: mutex
worth changing these to SpinLock, which I believe are a bit faster to lock/unlock?
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:
PS9, Line 188: Queue
> Could you specify which queue this is? will help people understand what's t
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 (#8).
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/8
--
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: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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) {
I think there's a pretty bad bug with how the signalling works.
Consider a queue with a limit of N. If the queue gets into a state where get_list has N elements and put_list has 0 elements with all of the producer threads waiting on it, then the caller to BlockingGet() will drain get_list before signalling put_cv, even though it should wake up a producer thread once there is space in the queue. Worse, it will only wake up one producer thread each time BlockingGet() is called, which means that one only producer thread can run at a time.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4350/1//COMMIT_MSG
Commit Message:
Line 29: BlockingQueue to using POSIX pthread primitives instead of boost
> Btw, this overhead seems to be related to the thread interruption support i
Maybe we should just implement our own wrapper class around pthread's condition variable?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 2:
Patch #2 reverts to using the boost library condition variable for now. Given the wide spread usage of it in the existing code base, it seems a bit too distracting to replace all of them in this patch.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue.
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/4350/2/be/src/util/blocking-queue.h
File be/src/util/blocking-queue.h:
Line 155:
> It's always racy as we never hold read_lock when reading the size of read_l
Ah, good point.
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?
Line 197: boost::mutex get_lock;
Shouldn't we align get_lock?
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?
PS3, Line 29: struct
class?
PS3, Line 29: __attribute__ ((aligned(64)))
What do you think about putting this in compiler-util.h as CACHE_ALIGN or something along those lines?
PS3, Line 61: cv
cv_
PS3, Line 62: ConditionVariable
This and the typedef shouldn't be necessary in C++, it automatically lets you use the short name of the class/struct.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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<boost::mutex>& lock,
> What does the return value mean?
Done
Line 50: void inline NotifyOne() { pthread_cond_signal(&cv_); }
> 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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue
......................................................................
Patch Set 5:
(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: ck_) /
> should this be total_put_wait_time_ (i.e. the last "put" field)? If we wan
Sure, we can use 'pad_' as point of division here. It seems like a bit of an overkill to have to come up with extra structs which make the code less readable.
PS7, Line 74: signal
> NotifyAll() is not used here to avoid ...
Done
Line 74: // avoid the race in which the writer may be signalled between when it checks
> ... when it calls Wait() in BlockingPut().
Done
PS7, Line 100: h HDFS scan no
> is this explanation specific to HDFS scan node? We also use this queue in e
Actually, I find this sentence a bit misleading after re-reading the comment again. Removed.
PS7, Line 171: changed once t
> why is this okay for the callers outside this class? we still have the spli
Yes, this can be split into SizeLocked() which requires "put_lock_" be held and externally facing Size().
In general, there is no guarantee the queue size will not change after the function Size() returns so there is really no "true" 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
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4026: Implement double-buffering for BlockingQueue
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Chen Huang <pa...@utexas.edu>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Juan Yu <jy...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>