Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201709220106.v8M16knv022632@ip-10-146-233-104.ec2.internal>
Date: Fri, 22 Sep 2017 01:06:46 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5844=3A_use_a_MemPool_for_expr_local_allocations=0A?=
In-Reply-To: <gerrit.1504974363000.I4ba5a7542ed90a49a4b5586c040b5985a7d45b61@gerrit.cloudera.org>
References: <gerrit.1504974363000.I4ba5a7542ed90a49a4b5586c040b5985a7d45b61@gerrit.cloudera.org>
Reply-To: tarmstrong@cloudera.com, impala-cr@cloudera.com, marcelk@gmail.com,
        reviews@impala.incubator.apache.org, dhecht@cloudera.com
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.14.2
Content-Type: multipart/alternative; boundary="91K0Qo6ao7w="; charset=UTF-8
archived-at: Fri, 22 Sep 2017 01:06:51 -0000

--91K0Qo6ao7w=
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Tim Armstrong has posted comments on this change=2E ( http://gerrit=2Ecloud=
era=2Eorg:8080/8025 )

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


Patch Set 6:

(4 comments)

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8=
025/6//COMMIT_MSG
Commit Message:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/=
8025/6//COMMIT_MSG@13
PS6, Line 13: * They are freed in bulk at various poi=
nts in query execution=2E
> we don't have to do it that way though=2E
Manag=
ing the expr allocations in bulk like this makes sense to me - trying to tr=
ack the lifetime of individual allocations would add a lot of complexity an=
d runtime overhead and I think also be a breaking changes for the UDF inter=
face=2E

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=2E

In a lot of cases it would be possible =
to free the local allocations more frequently, e=2Eg=2E after each expr eva=
luation (e=2Eg=2E for EvalConjuncts() where no var-len data can be returned=
) but I think that would add a lot of runtime overhead=2E


http://gerrit=
=2Ecloudera=2Eorg:8080/#/c/8025/6//COMMIT_MSG@31
PS6, Line 31: Remove Funct=
ionContext::ReallocateLocal()
> is that a UDF breaking change?
It was never=
 exposed in udf=2Eh so should be fine - it was only added to support String=
Val::Resize()


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8025/6/be/src/exec/=
exec-node=2Eh
File be/src/exec/exec-node=2Eh:

http://gerrit=2Ecloudera=2Eo=
rg:8080/#/c/8025/6/be/src/exec/exec-node=2Eh@324
PS6, Line 324: expr-manage=
d 
> what does that mean, especially given that it can be used by multiple =
evalu
Not sure of a better way of expressing it=2E

The lifetime of the mem=
ory is tied to the lifetime of the evaluators - it can only be freed when t=
he evaluators are closed since it backs the FreePools and a few other thing=
s=2E


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8025/6/be/src/runtime/free-p=
ool=2Eh
File be/src/runtime/free-pool=2Eh:

http://gerrit=2Ecloudera=2Eorg:=
8080/#/c/8025/6/be/src/runtime/free-pool=2Eh@a69
PS6, Line 69: 
> why does =
this go away?
It was moved to FunctionContext, which is the only user of Fr=
eePool=2E With the change to the local allocations coming from a MemPool th=
ey can't be intercepted here any more=2E


-- 
To view, visit http://gerri=
t=2Ecloudera=2Eorg:8080/8025
To unsubscribe, visit http://gerrit=2Ecloudera=
=2Eorg:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerr=
it-MessageType: comment
Gerrit-Change-Id: I4ba5a7542ed90a49a4b5586c040b5985=
a7d45b61
Gerrit-Change-Number: 8025
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Ar=
mstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@clou=
dera=2Ecom>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera=2Ecom>
Gerr=
it-Comment-Date: Fri, 22 Sep 2017 01:06:46 +0000
Gerrit-HasComments: Yes

--91K0Qo6ao7w=--