Tim Armstrong has posted comments on this change. Change subject: IMPALA-4397,IMPALA-3259: reduce codegen time and memory ......................................................................
Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/4956/7/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: PS7, Line 1706: if (aggregate_evaluators_.size() > Expr::CODEGEN_INLINE_EXPRS_THRESHOLD) { > The issue with heuristics is they may lead to performance cliffs in corner My main concern is adding an extra knob that has hard-to-understand behaviour. E.g. if we add an option to revert to the old behaviour globally, then it may fix one query but cause codegen to blow up on another query. Also it adds a burden to people maintaining the code in future to preserve the behaviour of the knob. There is the possibility of regressions, but not huge since this only kicks on for fairly large queries. E.g. for aggregations with 26+ simple aggregate functions we might see a 25% regression. I'm actually going to push a change that bumps that up to 100 - there are two levels of inlining, so I can just disable inlining of the outer function at the first threshold, which should have less impact. -- 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: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Silvius Rus <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
