You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/05/22 16:36:35 UTC
[Impala-ASF-CR] Parquet scanner microoptimizations
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6950
Change subject: Parquet scanner microoptimizations
......................................................................
Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-readers.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
6 files changed, 55 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/1
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 19:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/641/
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 10: Code-Review+1
Carry +1
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 9: Code-Review+1
Carry +1
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 17: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/636/
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#15).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/tuple.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
9 files changed, 117 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/15
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/8/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 991: InitTuple(template_tuple_, scratch_batch_->GetTuple(i));
does the extra branch in InitTuple not matter because it's always perfectly predicted? it would probably be good to make this path a bit faster as well, most large tables are partitioned.
why not an InitTuplesToTemplate(desc, template, Tuple* tuples, int num_tuples) {
for (< num_tuples; ++i, tuple += desc->byte_size()) {
memcpy(tuple, template, desc->byte_size());
}
}
or something like that.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6950/5/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 315: /// Use __restrict__ so that the compiler knows that it is safe to cache member
Just so I'm clear:
__restrict__ on the method implies that 'this' isn't aliased by any other pointers. However, 'num_values' already was assumed by the compiler not to alias 'this' because 'int' and 'decltype(*this)' do not match. On the other hand, uint8_t is the same as unsigned char on our platform, so 'tuple_mem' could alias 'this', so it also needs a __restrict__?
If that's right, I think the comment could expand a bit more to clarify this to future readers.
Line 489: uint8_t val_buf[sizeof(T)];
Also needs aligning as T, right?
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 15: Code-Review+2
Rebase
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6950/11/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 979: Status HdfsParquetScanner::ResetScratchBatch() {
> Looks like this is only called in a single place, so might as well move the
Done
http://gerrit.cloudera.org:8080/#/c/6950/11/be/src/exec/hdfs-scanner.h
File be/src/exec/hdfs-scanner.h:
Line 492: for (int i = 0; i < num_tuples; ++i) {
> int64_t i since num_tuples is an int64_t
Done
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/14/be/src/common/compiler-util.h
File be/src/common/compiler-util.h:
Line 46: /// Clang is incredibly pedantic about __restrict__ and doesn't offer an option to be
> Can you be more specific?
Done
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6950/5/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 315: /// Use __restrict__ so that the compiler knows that it is safe to cache member
> We compile with -fno-strict-alias so there's no type-based alias analysis a
And you can't __restrict__ num_values and pool because they actually can alias?
Line 489: void* slot = tuple->GetSlot(tuple_offset_);
> Seems like a good idea. I'm not sure if we really depend on it - generally
alignas(T)?
http://en.cppreference.com/w/cpp/language/alignas
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#13).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/tuple.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
8 files changed, 107 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/13
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#6).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
7 files changed, 77 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/6
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/5/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 315: /// Use __restrict__ so that the compiler knows that it is safe to cache member
> Those shouldn't alias either but there's not much value in caching them bec
For me it would be clearer to apply it on all pointers, but if you feel the opposite way I can understand.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/8/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 991: InitTuple(template_tuple_, scratch_batch_->GetTuple(i));
> does the extra branch in InitTuple not matter because it's always perfectly
Good point. I think the bottleneck there is probably more the branching inside memcpy(), since byte_size isn't a compile-time constant, but we might as well make this more efficient.
I moved the logic to an InitTupleBuffer() function in HdfsScanner and use a more optimised implementation for all three cases.
I did some basic profiling of scans of a partitioned table with perf top and it seems like most of the time is spent inside memcpy() both before and after the change.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 19: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Reviewed-on: http://gerrit.cloudera.org:8080/6950
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/tuple.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
9 files changed, 114 insertions(+), 47 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Tim Armstrong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 20
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/5/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 489: uint8_t val_buf[sizeof(T)];
> alignas(T)?
Oh that's better
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 14: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#9).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/tuple.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
9 files changed, 113 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/9
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker, Impala Public Jenkins, Jim Apple, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#17).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/tuple.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
9 files changed, 117 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/17
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "anujphadke (Code Review)" <ge...@cloudera.org>.
anujphadke has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 426: *num_values = (curr_tuple - tuple_mem) / tuple_size;
Should we add a DCHECK to make sure that tuple_size is non-zero?
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#3).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-readers.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
6 files changed, 56 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/3
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6950/4/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 979: Status HdfsParquetScanner::ResetScratchBatch() {
Why not move this into ScratchTupleBatch, i.e. pass in the template tuple there.
Line 983: if (template_tuple_ == nullptr && tuple_byte_size_ <= CACHE_LINE_SIZE) {
Not sure I completely understand the CACHE_LINE_SIZE check. We are zeroing scratch_capacity * tuple_byte_size_ bytes of contiguous memory. Why not remove the second condition?
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 19: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#10).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/hdfs-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/tuple.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
9 files changed, 118 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/10
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 16:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/632/
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6950/4/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 979: Status HdfsParquetScanner::ResetScratchBatch() {
> Why not move this into ScratchTupleBatch, i.e. pass in the template tuple t
ScratchTupleBatch then would have to call out to HdfsScanNode::InitTuple(). I can do a larger restructure, e.g. moving InitTuple() into Tuple or similar if you think that will make things clearer. I think it's probably an improvement - just checking that you think that makes sense before doing it.
Line 983: if (template_tuple_ == nullptr && tuple_byte_size_ <= CACHE_LINE_SIZE) {
> Not sure I completely understand the CACHE_LINE_SIZE check. We are zeroing
Augmented the comment.
There's some cut-over where the old code is faster. E.g. if the tuple has 1000 slots, it's probably better to zero out 125 bytes of null indicators row-by-row instead of zeroing out all the 1024 multi-kb rows.
I think this optimisation doesn't matter too much for tuples with more than a handful of slots, since the cost of materialization is high compared to the cost of zeroing things.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 14:
I did a dry run and it turns out clang was unhappy with a couple of the qualifiers.
I think clang's implementation of restrict is somewhat broken so as a workaround I made a macro to remove that qualifier for clang.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 17: Code-Review+2
Carry +2
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 16: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/632/
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 18: Code-Review+2
Revert the part of the change that assumed tuple_size > 0 - tests revealed the bug.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker, Impala Public Jenkins, Jim Apple, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#19).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/tuple.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
9 files changed, 114 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/19
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6950/5/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 315: /// Use __restrict__ so that the compiler knows that it is safe to cache member
> Just so I'm clear:
We compile with -fno-strict-alias so there's no type-based alias analysis at all - basically anything can alias. ParquetLevelDecoder::cached_levels_ is also a problem.
I expanded the comment to be less hand-wavy.
Line 489: uint8_t val_buf[sizeof(T)];
> Also needs aligning as T, right?
Seems like a good idea. I'm not sure if we really depend on it - generally we don't assume that Tuples are actually aligned.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 15: Code-Review+2
THank you!
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#5).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
7 files changed, 74 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/5
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 937: if (template_tuple_ == nullptr && tuple_byte_size_ <= CACHE_LINE_SIZE) {
> Might be worth putting this into an InitBatch() function.
Factored this out along with the above Reset() call, since they really go together.
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 486: uint8_t val_buf[sizeof(T)]; // Uninitialized stack allocation for temporary value.
> Comment just seems to describe what this line is doing, but not why. Mentio
Done
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/util/bit-stream-utils.inline.h
File be/src/util/bit-stream-utils.inline.h:
Line 91: ALWAYS_INLINE inline bool BitReader::GetValue(int num_bits, T* v) {
> remove inline?
I tried initially but it turns out the the always_inline attribute doesn't imply one-definition-rule linkage in the same way that "inline" - there was a compiler warning.
However, defining the function inside a class body implies "inline", so "inline" can be safely omitted in those cases.
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/util/dict-encoding.h
File be/src/util/dict-encoding.h:
Line 291: ALWAYS_INLINE inline bool DictDecoder<T>::GetNextValue(T* value) {
> remove inline?
The definition is outside the class so it's not actually redundant (see earlier comment).
Line 304: ALWAYS_INLINE inline bool DictDecoder<Decimal16Value>::GetNextValue(
> remove inline?
The definition is outside the class so it's not actually redundant (see earlier comment).
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/util/rle-encoding.h
File be/src/util/rle-encoding.h:
Line 251: ALWAYS_INLINE inline bool RleDecoder::Get(T* val) {
> remove inline?
The definition is outside the class so it's not actually redundant (see earlier comment).
Line 269: if (UNLIKELY(!bit_reader_.GetValue(bit_width_, val))) return false;
> Really a critical change? It changes the behavior, so might be dangerous /
It made a small but measurable difference, but probably not enough to worry about and there is an edge case where this is less correct so I reverted it.
I think it's generally safe - it doesn't change the external behaviour except where the input data is malformed and the client repeatedly calls Get() after it returns false. Then maybe this GetValue() call could return false, but the one on line 280 will later return true and return some RLE-encoded data.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#4).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
7 files changed, 74 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/4
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#8).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
7 files changed, 84 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/8
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 12: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6950/11/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 979: Status HdfsParquetScanner::ResetScratchBatch() {
Looks like this is only called in a single place, so might as well move the two lines there and remove this function.
http://gerrit.cloudera.org:8080/#/c/6950/11/be/src/exec/hdfs-scanner.h
File be/src/exec/hdfs-scanner.h:
Line 492: for (int i = 0; i < num_tuples; ++i) {
int64_t i since num_tuples is an int64_t
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/5/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 315: /// Use __restrict__ so that the compiler knows that it is safe to cache member
> And you can't __restrict__ num_values and pool because they actually can al
Those shouldn't alias either but there's not much value in caching them because the indirection doesn't seem to have much impact. I'm not sure if it's clearer to apply __restrict__ to all pointers where it would be valid, or to only apply it to those where it provides a performance benefit. What do you think would be clearer?
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#14).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/tuple.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
9 files changed, 115 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/14
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/4/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 979: Status HdfsParquetScanner::ResetScratchBatch() {
> Good point. The change may be an improvement, but I don't think we need to
I'm fine with deferring it
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/14/be/src/common/compiler-util.h
File be/src/common/compiler-util.h:
Line 46: /// Clang is incredibly pedantic about __restrict__ and doesn't offer an option to be
Can you be more specific?
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Marcel Kornacker, Impala Public Jenkins, Jim Apple, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#18).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Plain-encoded STRING: 7.07s -> 6.08s (most time spent in Snappy)
SELECT count(*) FROM biglineitem WHERE length(l_comment) > 37
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/common/compiler-util.h
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/runtime/tuple.h
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
9 files changed, 114 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/18
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 18
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 16: Code-Review+2
Rebase
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 16
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 17:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/636/
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 17
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/5/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 315: /// Use __restrict__ so that the compiler knows that it is safe to cache member
> For me it would be clearer to apply it on all pointers, but if you feel the
Done
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 426: *num_values = (curr_tuple - tuple_mem) / tuple_size;
> Should we add a DCHECK to make sure that tuple_size is non-zero?
Added it. It was already a precondition for the function (can't materialize a value into a 0-length tuple) so it's safe.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6950/4/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 979: Status HdfsParquetScanner::ResetScratchBatch() {
> ScratchTupleBatch then would have to call out to HdfsScanNode::InitTuple().
Good point. The change may be an improvement, but I don't think we need to do that now. In particular, rebasing my scanner change is not going to be fun (but I'll do it if you prefer to make the change now).
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#2).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-readers.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
6 files changed, 55 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/2
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6950
to look at the new patch set (#7).
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
IMPALA-5347: Parquet scanner microoptimizations
A mix of microoptimizations that profiling the parquet scanner revealed.
All lead to some measurable improvement and added up to significant
speedups for various scans.
* Add ALWAYS_INLINE to hot functions that GCC was mistakenly not inlining
in all cases.
* Apply __restrict__ in a few places so the compiler knows that it is
safe to cache values accessed via those pointers
* memset() the whole batch instead of the null indicators is cases where
it is almost certainly cheaper.
* Avoid updating two correlated loop variables in MaterializeValueBatch().
* Avoid unnecessary initialization of often-unused 'val' in ReadSlot().
* Shave a few instructions off the (still very expensive) bit unpacking
and dict decoding logic.
Performance:
Some local TPC-H and targeted-perf benchmarks showed average speedups of
~5%.
I did some benchmarks targeted at measuring column materialisation
performance using a version of lineitem with duplicated data to make
it bigger. These queries all got significantly faster.
Dict-encoded DECIMAL: 2.23 -> 1.23s
SELECT count(*) FROM biglineitem WHERE l_quantity > 49
Plain-encoded BIGINT: 2.33s -> 1.62s
SELECT count(*) FROM biglineitem WHERE l_orderkey != 10
Dict-encoded STRING: 2.73s -> 1.72s
SELECT count(*) FROM biglineitem WHERE l_returnflag = 'A'
Multiple columns: 5.15s -> 3.74s
SELECT count(*) FROM biglineitem
WHERE l_quantity > 49 and l_partkey != 199 and l_tax < 100
Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-parquet-scanner.h
M be/src/exec/parquet-column-readers.cc
M be/src/util/bit-stream-utils.inline.h
M be/src/util/bit-util.h
M be/src/util/dict-encoding.h
M be/src/util/rle-encoding.h
7 files changed, 80 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/50/6950/7
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Mostafa Mokhtar <mm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
[Impala-ASF-CR] IMPALA-5347: Parquet scanner microoptimizations
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5347: Parquet scanner microoptimizations
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
Line 937: if (template_tuple_ == nullptr && tuple_byte_size_ <= CACHE_LINE_SIZE) {
Might be worth putting this into an InitBatch() function.
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:
Line 486: uint8_t val_buf[sizeof(T)]; // Uninitialized stack allocation for temporary value.
Comment just seems to describe what this line is doing, but not why. Mention explicitly what we are trying to avoid for a perf gain.
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/util/bit-stream-utils.inline.h
File be/src/util/bit-stream-utils.inline.h:
Line 91: ALWAYS_INLINE inline bool BitReader::GetValue(int num_bits, T* v) {
remove inline?
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/util/dict-encoding.h
File be/src/util/dict-encoding.h:
Line 291: ALWAYS_INLINE inline bool DictDecoder<T>::GetNextValue(T* value) {
remove inline?
Line 304: ALWAYS_INLINE inline bool DictDecoder<Decimal16Value>::GetNextValue(
remove inline?
http://gerrit.cloudera.org:8080/#/c/6950/2/be/src/util/rle-encoding.h
File be/src/util/rle-encoding.h:
Line 251: ALWAYS_INLINE inline bool RleDecoder::Get(T* val) {
remove inline?
Line 269: if (UNLIKELY(!bit_reader_.GetValue(bit_width_, val))) return false;
Really a critical change? It changes the behavior, so might be dangerous / introduce a bug.
--
To view, visit http://gerrit.cloudera.org:8080/6950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49ec523a65542fdbabd53fbcc4a8901d769e5cd5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: anujphadke <ap...@cloudera.com>
Gerrit-HasComments: Yes