impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5844: use a MemPool for expr local allocations
Date Fri, 22 Sep 2017 16:58:08 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Sep 2017 16:58:08 +0000
Gerrit-HasComments: Yes

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