impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5520: TopN node periodically reclaims old allocations
Date Wed, 12 Jul 2017 00:37:16 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5520: TopN node periodically reclaims old allocations
......................................................................


Patch Set 1:

(4 comments)

I didn't do a full pass, but I had some comments about the codegen and memory management parts
of this.

http://gerrit.cloudera.org:8080/#/c/7400/1/be/src/exec/topn-node-ir.cc
File be/src/exec/topn-node-ir.cc:

Line 61: void TopNNode::ReclaimTuplePool() {
I don't think this needs to be in the *-ir.cc - the stuff in here gets recompiled for every
query, so we try not to put anything in here that doesn't benefit from codegen.

I think we can move ReclaimTuplePool() and the call to it above to topn-node.cc.


Line 62:   auto temp_pool = new MemPool(mem_tracker());
It would be good to use unique_ptr here just so we don't accidentally create a memory leak
later. It won't be leaked now but when you switch Allocate() to TryAllocate() it gets trickier.
Also when we eventually modify DeepCopy to also return a bool or Status.


Line 63:   auto temp_pq = new std::priority_queue<Tuple*, std::vector<Tuple*>,
Rebuilding the priority queue like this seems unnecessary. It's a std::vector internally so
there's no fundamental reason we can't update the tuples in-place.

https://stackoverflow.com/a/12886393 gives two ways to do this - either subclass it or use
std::vector and std::push_heap/pop_heap etc. Either seems OK to me. I probably prefer the
push_heap approach.


PS1, Line 69: Allocate
We're trying to move to using TryAllocate() instead and failing synchronously when we run
out of memory.


-- 
To view, visit http://gerrit.cloudera.org:8080/7400
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I968f57f0ff2905bd581908bc5c5ee486b31e6aa8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message