Michael Ho has posted comments on this change. Change subject: IMPALA-3332: Free local allocations in sorter. ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/3109/1/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: Line 353: Less > how about "CompareLess"? I hope you don't mind if I keep it as Less for two reasons: 1. some call sites can fit in one line instead of two lines so the code is easier to read IMHO. 2. this matches the name of the function it wraps around (i.e. comparator_.Less). Line 356: error > an error status Done Line 366: error > an error status Done Line 894: bool is_less = comparator_.Less(lhs, rhs); > This might be more concise if we call Less() after doing the comparison_cou Done. I just find it easier to reason about if all outstanding locations are freed here but then again, it's no difference from the case in which the number of comparison is not a multiple of batch size. Line 897: state_->batch_size() > Maybe cache this, or just use a sensible constant like 1024? Maybe it doesn Yes, I also wondered if we should just use DEFAULT_BATCH_SIZE instead but all other exec nodes would have freed local allocations once per row batch so I am a bit hesitant. I have rewritten the code to do decrement instead. That will allow us to call state->batch_size() once per batch size. Line 897: state_->batch_size() > or you could make it a count down so that it's only loaded on the unlikely Yup, that's exactly the same approach I take in the new patch. Line 936: if (UNLIKELY(state_->is_cancelled())) return Status::CANCELLED; > We insertion sort max 16 elements, so I think we can elide these checks. Good point. Moved the checks out of the loop. Line 948: while (true) { > This loop can go on a long time - it does a pass over the entire buffered i Good point. Added a check per iteration. http://gerrit.cloudera.org:8080/#/c/3109/1/be/src/util/tuple-row-compare.h File be/src/util/tuple-row-compare.h: Line 136: > Extra line. Removed. http://gerrit.cloudera.org:8080/#/c/3109/1/testdata/workloads/functional-query/queries/QueryTest/sort.test File testdata/workloads/functional-query/queries/QueryTest/sort.test: Line 4112: # periodically. disable_outermost_topn=1 forces sort node to be used. > There are a number of tests in spilling.test that have mem_limits commented Will try that in the next patch. -- To view, visit http://gerrit.cloudera.org:8080/3109 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id658243d4ffa9dcdbf4e867db0bb3e8d161af086 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
