impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3286: Software prefetching for hash table build.
Date Fri, 29 Apr 2016 01:08:11 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message