Michael Ho has posted comments on this change. Change subject: IMPALA-3286: Software prefetching for hash table build. ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/2896/1/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: Line 864: 12 > Hmm this is the index of the field in the class? Yes, that's the index. Updated based on your suggestion. Also moved the field around so that its neighbor won't be of type TupleRow* so in case we screw up, LLVM will notice the wrong type in the icmp below. http://gerrit.cloudera.org:8080/#/c/2896/1/be/src/exec/hash-table.h File be/src/exec/hash-table.h: Line 295: /// for matches. It's not evaluated if probing doesn't call Equals(). > There seems to be some logic that if this is NULL, it won't be reevaluated Done http://gerrit.cloudera.org:8080/#/c/2896/1/be/src/exec/hash-table.inline.h File be/src/exec/hash-table.inline.h: Line 122: PrefetchBucket > PrefetchBucketForWrite()? To make it clear the flags to __builtin_prefetch Done http://gerrit.cloudera.org:8080/#/c/2896/1/be/src/exec/partitioned-hash-join-node-ir.cc File be/src/exec/partitioned-hash-join-node-ir.cc: Line 313: FOREACH_ROW(prefetch_batch, 0, row) { > Prefetching 1024 rows at a time seems suboptimal. If the cache line is 64 b Yes, I think we should experiment with different prefetch size. Added a TODO statement. Ideally, this should be something configured at Impala startup time based on the CPU and then used for constant substitution at codegen time. Line 329: ht_ctx->SetRowToEval(row); > The logic with SetRowToEval() is kind of tricky - maybe it needs a comment Comment added. -- To view, visit http://gerrit.cloudera.org:8080/2896 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib85e7fc162ad25c849b9e716b629e226697cd940 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
