Tim Armstrong has posted comments on this change. Change subject: IMPALA-3286: prefetching for PartitionedAggregationNode ......................................................................
Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/3070/5/be/src/exec/partitioned-aggregation-node-ir.cc File be/src/exec/partitioned-aggregation-node-ir.cc: Line 50: std::min<int>(batch->num_rows() > doesn't the FOREACH_ROW_LIMIT take care of this? besides, it doesn't really Sure, but we need to keep track of the start of the next batch somehow. This seemed to express the intent more directly than relying on FOREACH_ROW_LIMIT and using an additional counter. The final group gets handled ok even if it's not a multiple by the FOREACH_ROW_LIMIT macro. I realise now that taking the min() with num_rows is redundant so I'll remove it.. Line 50: batch_size > cache_size Done Line 64: Batch > Since we're not doing the whole batch, maybe rename this to something like Done Line 68: batch_size > cache_size Done Line 79: // Hoist lookups out of non-null branch to speed up non-null case. > why does this speed it up? I didn't dig down deep enough to fully understand - it probably gives the compiler/cpu more freedom to schedule instructions. Line 82: HashTable* hash_tbl = hash_partitions_[partition_idx]->hash_tbl.get(); We added a fair bit of extra pointer chasing here: PartitionedAggregationNode -> hash_partitions_ -> internal vector<> array -> Partition -> HashTable -> HashTable::buckets_ This seems to hurt on low-ndv aggs. So to mitigate the regression I added a cache for hash_tbls_ that short-circuits that chain of pointers. Line 182: const int batch_size = std::min<int>(num_rows, expr_vals_cache->capacity()); > same comments Done http://gerrit.cloudera.org:8080/#/c/3070/5/be/src/exec/partitioned-aggregation-node.h File be/src/exec/partitioned-aggregation-node.h: Line 490: free space available > capacity of (i.e. the cache always starts empty). Done Line 558: /// that 'prefetch_mode' will be substituted with constants during codegen time. > maybe move after needs_serialize to keep in order Done -- To view, visit http://gerrit.cloudera.org:8080/3070 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7726454efb416d61080c4e11db0ee7ada18c149b Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
