impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4397,IMPALA-3259: reduce codegen time and memory
Date Wed, 23 Nov 2016 01:41:27 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4397,IMPALA-3259: reduce codegen time and memory
......................................................................


Patch Set 11:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/4956/11/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:

Line 939:   if (is_corrupt_) {
> did the formatter do this?
Oops, changed it back.


Line 988:                                               bytes_allocated),
> odd indentation
This was clang-format. I split it into two statements to avoid the weirdness.


Line 1044:                                               estimated_memory),
> odd indentation
This was clang-format. I split it into two statements to avoid the weirdness.


http://gerrit.cloudera.org:8080/#/c/4956/11/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

Line 127: //
> augment with brief comment describing memory tracking.
Done


Line 467:   /// expressions), avoid inlining the individual expression evaluation into the
> could we make it "avoid inlining for the exprs exceeding this threshold"?
Done


Line 595:   void DestroyIRModule();
> DestroyIrModule
Done


http://gerrit.cloudera.org:8080/#/c/4956/11/be/src/exec/hash-join-node.cc
File be/src/exec/hash-join-node.cc:

Line 140:                                 false, std::logical_or<bool>());
> odd indentation
Reverted it. It may be difficult to prevent clang-format for doing this again though.


http://gerrit.cloudera.org:8080/#/c/4956/11/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

Line 302:   if (IsNodeCodegenDisabled()) return;
> i find this counterintuitive - why call codegen() if it's disabled (ie, i'd
ExecNode::Codegen() is the function that walks the children nodes to Codegen() them, so we
need to call it. We could refactor it so that we have a separate function called CodegenChildren()
and ExecNode::Codegen() just calls that, but it's unclear if it's worth adding more indirection.


http://gerrit.cloudera.org:8080/#/c/4956/11/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:

Line 824:       mergeFragment.getPlanRoot().setDisableCodegen(true);
> what is this for? does exchange ever do codegen?
Non-merging exchanges don't currently (we could codegen some of the pointer swizzling). I
just added this because in principle we know it has the same number of rows flowing through
it as the aggregation. Otherwise there's an assumption baked in here that the backend for
ExchangeNode doesn't support codegen.


-- 
To view, visit http://gerrit.cloudera.org:8080/4956
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id10015b49da182cb181a653ac8464b4a18b71091
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Silvius Rus <srus@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message