impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5844: use a MemPool for expr local allocations
Date Fri, 22 Sep 2017 01:06:46 GMT
Tim Armstrong has posted comments on this change. ( )

Change subject: IMPALA-5844: use a MemPool for expr local allocations

Patch Set 6:

Commit Message:
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.
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()
File be/src/exec/exec-node.h:
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.
File be/src/runtime/free-pool.h:
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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Fri, 22 Sep 2017 01:06:46 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message