Tim Armstrong has posted comments on this change. Change subject: IMPALA-3115: Hoist some variables out of loops in cross-compiled code. ......................................................................
Patch Set 2: (3 comments) Looks good to me. Disappointing that there weren't bigger speedups but I think having the cleaner IR will help a lot with performance work going forward. I'd be interested in seeing the benchmark reports if they're available. When I did a hacky version of this a while back I saw a couple of good speedups on TPCH, I wonder if some intervening change made it so that cached loads/stores were less of a bottleneck: a more recent benchmark run show both of those queries faster than the baseline. +-----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+ | Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Num Clients | Iters | +-----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+ | TPCH(_20) | TPCH-Q13 | parquet / none / none | 11.24 | 14.67 | I -23.37% | 2.04% | 2.63% | 1 | 10 | | TPCH(_20) | TPCH-Q17 | parquet / none / none | 27.97 | 36.92 | I -24.23% | 1.76% | 0.56% | 1 | 10 | http://gerrit.cloudera.org:8080/#/c/2661/2/be/src/exec/partitioned-aggregation-node-ir.cc File be/src/exec/partitioned-aggregation-node-ir.cc: Line 195: const HashTableCtx* __restrict__ ht_ctx, Partition* __restrict__ partition, I think these restrict qualifiers may be discarded when TryAddToHashTable is inlined into ProcessBatchStreaming: did they have an effect? http://gerrit.cloudera.org:8080/#/c/2661/2/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 274: buildStatus_ while you're here, make it build_status_? http://gerrit.cloudera.org:8080/#/c/2661/2/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: Line 145: /// For row batches with zero capacity, Next() will always return NULL. Maybe we should just DCHECK if Next() is called on an empty batch? The code already assumes that the caller checks capacity in the non-empty case. -- To view, visit http://gerrit.cloudera.org:8080/2661 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic7152b1fb094b3c3574d203e3774f4297f2225dc Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
