Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8025 )
Change subject: IMPALA-5844: use a MemPool for expr local allocations ...................................................................... Patch Set 6: (3 comments) I guess my vote is for expr_perm_mem_pool_ and expr_results_mem_pool_. What do you think? kinda long though, should we remove "mem"? http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/exec/exec-node.h@324 PS6, Line 324: expr-managed > Yeah that makes sense to me. Maybe tmp_mem_pool_ and perm_mem_pool_ or expr Okay, keeping this specific to exprs, at least for now is fine, so that we don't have to muck around with the tracking. But let's clarify the lifetime of this pool, i.e. ExecNode Prepare() to Close() or whatever. I guess we can go with expr_perm_mem_pool_. http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/exec/exec-node.h@333 PS6, Line 333: expr_tmp_mem_pool_ let's go with expr_results_mem_pool_. And then I think we should explain it in terms of being the place where results are passed back, and thus has the lifetime of the results. And maybe we even clarify to say results of GetValue()? http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/exprs/scalar-expr-evaluator.h File be/src/exprs/scalar-expr-evaluator.h: http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/exprs/scalar-expr-evaluator.h@74 PS6, Line 74: Evaluator-managed allocations from this : /// evaluator will be from 'expr_mem_pool' while local allocations will be from : /// 'local_mem_pool'. > that seems pretty tough to understand and distinguish. what does "evaluato And then here I guess, from the perspective of the evaluator, we call them perm_mem_pool_ and results_mem_pool_, and basically explain that perm is guaranteed to have a lifetime at least as long as the evaluator itself, while result pool lifetime is controlled by the caller. -- 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: 6 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 22 Sep 2017 23:15:45 +0000 Gerrit-HasComments: Yes
