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-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:


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

Line 61: void TopNNode::ReclaimTuplePool() {
I don't think this needs to be in the * - 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

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. 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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I968f57f0ff2905bd581908bc5c5ee486b31e6aa8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bikramjeet Vig <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message