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....@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
