Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8025 )
Change subject: IMPALA-5844: use a MemPool for expr result allocations ...................................................................... Patch Set 12: (6 comments) Rebase, then addressed comments. http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/exec-node.h@379 PS11, Line 379: Status QueryMaintenance(RuntimeState* state) WARN_UNUSED_RESULT; > Yup. Mind dropping a TODO: IMPALA-2399 remove this. Done http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.cc@600 PS11, Line 600: > Cool. Alternatively, if it were worth it, then maybe it'd be better for Cle Clear() looks fairly cheap (it just iterates over the vector<ChunkInfo>) and I don't think this class is super perf critical - it's only called to clean up partitions that weren't fully processed. http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/scanner-context.h File be/src/exec/scanner-context.h: http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/scanner-context.h@337 PS11, Line 337: /// single-threaded scan node implementation. > Once we can get rid of the multi-threaded scanner case, this seems worth re IMPALA-6015 http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/runtime/sorter.h File be/src/runtime/sorter.h: http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/runtime/sorter.h@199 PS11, Line 199: /// MemPool for allocating data structures used by expression evaluators in the sorter. : MemPool expr_perm_pool_; : : /// MemPool for allocations that hold results of expression evaluation in the sorter. : /// Cleared periodically during sorting to prevent memory accumulating. : MemPool expr_results_pool_; : : /// In memory sorter and less-than comparator. : TupleRowComparator compare_less_than_; > Was also asking about the MemPool (e.g. in ExecNode we have indirection - i I think the reasoning there is similar - the MemTracker* is a constructor parameter and the MemTrackers are not set up until Prepare(). Another advantage is that we can forward-declare MemPool instead of pulling in the full header. http://gerrit.cloudera.org:8080/#/c/8025/12/be/src/udf/udf.h File be/src/udf/udf.h: http://gerrit.cloudera.org:8080/#/c/8025/12/be/src/udf/udf.h@352 PS12, Line 352: Serialize() > and Finalize()? or is it suppose to just say Serialize() is? Done http://gerrit.cloudera.org:8080/#/c/8025/12/be/src/udf/udf.h@356 PS12, Line 356: tringVal(FunctionContext*, int) ctor or StringVal::CopyFrom(FunctionContext*, : /// const uint8_t*, size_t). StringVal::Resize() also should be listed here. -- To view, visit http://gerrit.cloudera.org:8080/8025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4ba5a7542ed90a49a4b5586c040b5985a7d45b61 Gerrit-Change-Number: 8025 Gerrit-PatchSet: 12 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 04 Oct 2017 19:24:50 +0000 Gerrit-HasComments: Yes
