impala-reviews mailing list archives

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

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


Patch Set 4:

(15 comments)

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

Line 952:     RETURN_IF_ERROR(OptimizeModule());
Is there a reason why we don't call DestroyIRModule() when OptimizeModule() fails ?


PS4, Line 986:  memory_manager_->bytes_allocated()),
             :         memory_manager_->bytes_allocated()
The code may be easier to read if this factored into a single variable instead.


Line 1039:   if (!mem_tracker_->TryConsume(estimated_memory)) {
I wonder if it's better to skip optimization if we cannot reserve the memory to do so than
punting back to interpretation ?


PS4, Line 1041:         Substitute("Codegen failed to reserve '$0' bytes for optimization",
              :                                               estimated_memory),
              :         estimated_memory);
These line wraps look a bit weird to me but not sure if it's a side effect of clang-format
?


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

PS4, Line 708: optimiser
nit: optimizer


PS4, Line 709:  512 bytes
Mind documenting how this is derived so we can update it accordingly in case we change the
optimization level in the future ?


http://gerrit.cloudera.org:8080/#/c/4956/4/be/src/codegen/mcjit-mem-mgr.h
File be/src/codegen/mcjit-mem-mgr.h:

PS4, Line 36: memory
This is for tracking memory consumed by the compiled machine code, right ?


http://gerrit.cloudera.org:8080/#/c/4956/4/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

PS4, Line 187:  false
true ?


http://gerrit.cloudera.org:8080/#/c/4956/4/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

PS4, Line 762:     if (ctxs.size() > Expr::CODEGEN_INLINE_EXPRS_THRESHOLD) {
             :       // Avoid bloating function by inlining too many exprs into it.
             :       expr_fn->addFnAttr(llvm::Attribute::NoInline);
             :     }
I wonder if this can be more generalized by putting it in LlvmCodegen::FinalizeFunction()
instead and use instruction count to determine whether the function is too large to be worth
being inlined.

Of course, this may be a problem if we intentionally inline a big function  into the cross-compiled
code and rely on constant folding to eliminate 90% of the instructions in the function. In
which case, we may need to do a quick constant propagation pass first in FinalizeFunction().
Just a thought.


http://gerrit.cloudera.org:8080/#/c/4956/4/be/src/exec/hdfs-scanner.h
File be/src/exec/hdfs-scanner.h:

PS4, Line 190: CODEGEN_WRITE_TUPLES_MAX_COLUMNS = 250;
This seems a bit adhoc to me. May be better to track the size of IR functions and don't use
it if it gets too large. Empirically, the time to generate the IR is rather small compared
to the optimization time so the wasted work may not be too costly and hopefully, the large
IR function is not a common case.


PS4, Line 190:  const
constexpr


http://gerrit.cloudera.org:8080/#/c/4956/4/be/src/exec/topn-node.cc
File be/src/exec/topn-node.cc:

PS4, Line 85: 
Oops. My bad.


http://gerrit.cloudera.org:8080/#/c/4956/4/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

PS4, Line 1911: 
              : 
Is it necessary to remove it ? For comparison, please see FirstValUpdate().


http://gerrit.cloudera.org:8080/#/c/4956/4/be/src/exprs/expr.h
File be/src/exprs/expr.h:

PS4, Line 311: const
constexpr ?


PS4, Line 311: CODEGEN_INLINE_EXPRS_THRESHOLD
Wouldn't instruction count be a more precise estimation ?


-- 
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: 4
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: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message