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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Silvius Rus <s...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
