Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201710041924.v94JOoct001433@ip-10-146-233-104.ec2.internal>
Date: Wed, 4 Oct 2017 19:24:50 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Michael Ho <kwho@cloudera.com>
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5844=3A_use_a_MemPool_for_expr_result_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,
        kwho@cloudera.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="byqiSloNs2Q="; charset=UTF-8
archived-at: Wed, 04 Oct 2017 19:24:56 -0000

--byqiSloNs2Q=
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 =
result 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 12:

(6 comments)

Rebase, then addressed comments=2E

h=
ttp://gerrit=2Ecloudera=2Eorg:8080/#/c/8025/11/be/src/exec/exec-node=2Eh
Fi=
le be/src/exec/exec-node=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8025=
/11/be/src/exec/exec-node=2Eh@379
PS11, Line 379:   Status QueryMaintenance=
(RuntimeState* state) WARN_UNUSED_RESULT;
> Yup=2E Mind dropping a TODO: IM=
PALA-2399 remove this=2E 
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/80=
25/11/be/src/exec/partitioned-aggregation-node=2Ecc
File be/src/exec/partit=
ioned-aggregation-node=2Ecc:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8025/=
11/be/src/exec/partitioned-aggregation-node=2Ecc@600
PS11, Line 600: 
> Coo=
l=2E Alternatively, if it were worth it, then maybe it'd be better for Cle
=
Clear() looks fairly cheap (it just iterates over the vector<ChunkInfo>) an=
d I don't think this class is super perf critical - it's only called to cle=
an up partitions that weren't fully processed=2E


http://gerrit=2Ecloudera=
=2Eorg:8080/#/c/8025/11/be/src/exec/scanner-context=2Eh
File be/src/exec/sc=
anner-context=2Eh:

http://gerrit=2Ecloudera=2Eorg:8080/#/c/8025/11/be/src/=
exec/scanner-context=2Eh@337
PS11, Line 337:   /// single-threaded scan nod=
e implementation=2E
> Once we can get rid of the multi-threaded scanner cas=
e, this seems worth re
IMPALA-6015


http://gerrit=2Ecloudera=2Eorg:8080/#/=
c/8025/11/be/src/runtime/sorter=2Eh
File be/src/runtime/sorter=2Eh:

http:/=
/gerrit=2Ecloudera=2Eorg:8080/#/c/8025/11/be/src/runtime/sorter=2Eh@199
PS1=
1, Line 199:   /// MemPool for allocating data structures used by expressio=
n evaluators in the sorter=2E
              :   MemPool expr_perm_pool_;
  =
            : 
              :   /// MemPool for allocations that hold resu=
lts of expression evaluation in the sorter=2E
              :   /// Cleared=
 periodically during sorting to prevent memory accumulating=2E
            =
  :   MemPool expr_results_pool_;
              : 
              :   /// In=
 memory sorter and less-than comparator=2E
              :   TupleRowCompar=
ator compare_less_than_;
> Was also asking about the MemPool (e=2Eg=2E in E=
xecNode we have indirection - i
I think the reasoning there is similar - th=
e MemTracker* is a constructor parameter and the MemTrackers are not set up=
 until Prepare()=2E Another advantage is that we can forward-declare MemPoo=
l instead of pulling in the full header=2E


http://gerrit=2Ecloudera=2Eorg=
:8080/#/c/8025/12/be/src/udf/udf=2Eh
File be/src/udf/udf=2Eh:

http://gerri=
t=2Ecloudera=2Eorg:8080/#/c/8025/12/be/src/udf/udf=2Eh@352
PS12, Line 352: =
Serialize()
> and Finalize()? or is it suppose to just say Serialize() is?
=
Done


http://gerrit=2Ecloudera=2Eorg:8080/#/c/8025/12/be/src/udf/udf=2Eh@3=
56
PS12, Line 356: tringVal(FunctionContext*, int) ctor or StringVal::CopyF=
rom(FunctionContext*,
              : /// const uint8_t*, size_t)=2E
String=
Val::Resize() also should be listed here=2E


-- 
To view, visit http://ge=
rrit=2Ecloudera=2Eorg:8080/8025
To unsubscribe, visit http://gerrit=2Ecloud=
era=2Eorg:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
G=
errit-MessageType: comment
Gerrit-Change-Id: I4ba5a7542ed90a49a4b5586c040b5=
985a7d45b61
Gerrit-Change-Number: 8025
Gerrit-PatchSet: 12
Gerrit-Owner: Ti=
m Armstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Reviewer: Dan Hecht <dhecht@=
cloudera=2Ecom>
Gerrit-Reviewer: Michael Ho <kwho@cloudera=2Ecom>
Gerrit-Re=
viewer: Tim Armstrong <tarmstrong@cloudera=2Ecom>
Gerrit-Comment-Date: Wed,=
 04 Oct 2017 19:24:50 +0000
Gerrit-HasComments: Yes

--byqiSloNs2Q=--