Tim Armstrong 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: (4 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. > we don't have to do it that way though. Managing the expr allocations in bulk like this makes sense to me - trying to track the lifetime of individual allocations would add a lot of complexity and runtime overhead and I think also be a breaking changes for the UDF interface. The operator code is the only code that knows when the results of expr evaluation are no longer needed so it makes sense to me that it would be responsible for freeing them. In a lot of cases it would be possible to free the local allocations more frequently, e.g. after each expr evaluation (e.g. for EvalConjuncts() where no var-len data can be returned) but I think that would add a lot of runtime overhead. http://gerrit.cloudera.org:8080/#/c/8025/6//COMMIT_MSG@31 PS6, Line 31: Remove FunctionContext::ReallocateLocal() > is that a UDF breaking change? It was never exposed in udf.h so should be fine - it was only added to support StringVal::Resize() 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 > what does that mean, especially given that it can be used by multiple evalu Not sure of a better way of expressing it. The lifetime of the memory is tied to the lifetime of the evaluators - it can only be freed when the evaluators are closed since it backs the FreePools and a few other things. http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/runtime/free-pool.h File be/src/runtime/free-pool.h: http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/runtime/free-pool.h@a69 PS6, Line 69: > why does this go away? It was moved to FunctionContext, which is the only user of FreePool. With the change to the local allocations coming from a MemPool they can't be intercepted here any more. -- 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 01:06:46 +0000 Gerrit-HasComments: Yes
