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) http://gerrit.cloudera.org:8080/#/c/8025/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8025/6//COMMIT_MSG@13 PS6, Line 13: * They are freed in bulk at various points in query execution. > Managing the expr allocations in bulk like this makes sense to me - trying I'm not suggesting we can't manage them in bulk for convenience. But what I meant is the old code was saving a pointer to each allocation at allocate time, and then calling Free() on each pointer at bulk free time. So, it doesn't seem to be cheaper than just calling Free() directly. That's why I don't see this as a "property" per-se. (I think the "owned and managed" bullet covers the requirements of lifetime and influences that choice.) Anyway, I don't think it really changes anything, so fine to leave the bullet. http://gerrit.cloudera.org:8080/#/c/8025/6//COMMIT_MSG@31 PS6, Line 31: Remove FunctionContext::ReallocateLocal() > It was never exposed in udf.h so should be fine - it was only added to supp Oh, I was confused because it says FunctionContext not FunctionContextImpl. 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 > Not sure of a better way of expressing it. I was reading it to say that the mem-pool was managed by the exprs, but I guess what you're saying is that the allocations made from this mem-pool are managed by the exprs. What I'm getting at is should we define these pools based on their lifetimes rather than their contents? I mean, in order to choose a pool to allocate from, should we just pick the pool with the right lifetime? Or do we actually want to have separate pools for allocating different kinds of things? maybe we do want different pools for mem tracking? Maybe easiest to discuss this in person. In any case, I think it would help to just say: Lifetime is from Prepare() to Close(). or something like that. -- 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 16:58:08 +0000 Gerrit-HasComments: Yes
