Bikramjeet Vig has posted comments on this change. Change subject: IMPALA-5520: TopN node periodically reclaims old allocations ......................................................................
Patch Set 1: (19 comments) http://gerrit.cloudera.org:8080/#/c/7400/1//COMMIT_MSG Commit Message: PS1, Line 9: tuple > in a tuple pool which is held longer than necessary, resulting in unnecessa Done PS1, Line 11: With this commit TopN node > commit, the TopN node Done PS1, Line 12: its > stored in the priority queue Done PS1, Line 12: This is done every time the : number of old rows (removed from the priority queue it uses) is more : than twice the row limit (N in TopN) > This is done when the number of rows removed from the priority queue is mor Done PS1, Line 24: general > expected general case Done PS1, Line 27: data > example worst case: data stored... Done PS1, Line 39: performance > query runtimes Done PS1, Line 39: More extensive perf testing will be : done in the future to recognize pathological cases > Not necessarily pathological cases, more that we're gonna do cluster testin Done http://gerrit.cloudera.org:8080/#/c/7400/1/be/src/exec/topn-node-ir.cc File be/src/exec/topn-node-ir.cc: PS1, Line 28: limit_ > why isn't this (limit_ + offset_), since that's the size of the PQ? Done Line 61: void TopNNode::ReclaimTuplePool() { > I don't think this needs to be in the *-ir.cc - the stuff in here gets reco Done Line 62: auto temp_pool = new MemPool(mem_tracker()); > It would be good to use unique_ptr here just so we don't accidentally creat Done 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::vect Done PS1, Line 69: Allocate > We're trying to move to using TryAllocate() instead and failing synchronous Done http://gerrit.cloudera.org:8080/#/c/7400/1/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: PS1, Line 88: NumOfTimesTuplePoolReclaimed > TuplePoolReclamations is more concise Done PS1, Line 88: > indentation Done http://gerrit.cloudera.org:8080/#/c/7400/1/be/src/exec/topn-node.h File be/src/exec/topn-node.h: PS1, Line 71: Create new tuple pool with only the elements inside the priority queue and release : // memory from the old tuple pool > Re-materialize the elements in the priority queue into a new tuple pool, an Done PS1, Line 115: poped > popped has two 'p's. Regardless, let's call this rows_to_reclaim_ because n Done http://gerrit.cloudera.org:8080/#/c/7400/1/tests/query_test/test_tpch_queries.py File tests/query_test/test_tpch_queries.py: Line 67: return 'tpch' > newline after this Done PS1, Line 75: \ > nit: remove space to be consistent Done -- 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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
