Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10679 )
Change subject: IMPALA-3816: (prep) Move TupleSorter to sorter-ir.cc ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-ir.cc File be/src/runtime/sorter-ir.cc: http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-ir.cc@28 PS3, Line 28: void Sorter::TupleIterator::NextPage > Does this need to be cross-compiled ? See my comment at L51. http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-ir.cc@39 PS3, Line 39: void Sorter::TupleIterator::PrevPage > Does this need to be cross-compiled ? See my comment at L51. http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-ir.cc@51 PS3, Line 51: void Sorter::TupleIterator::Next( > Does this need to be cross-compiled ? It doesn't need to be. But it's in the inner-most while loop in Partition() so we'd better inline it. Plus the IR code size increment is marginal so I don't see why not. http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-ir.cc@185 PS3, Line 185: Tuple* Sorter::TupleSorter::SelectPivot(TupleIterator begin, TupleIterator end) { > Does this need to be cross-compiled ? It calls Less(). See my comment at L209. http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-ir.cc@209 PS3, Line 209: Tuple* Sorter::TupleSorter::MedianOfThree(Tuple* t1, Tuple* t2, Tuple* t3) { > Does this need to be cross-compiled ? This calls Less(). After my change Less() will always call CompareInterpreted() if it's not replaced by the hand-crafted Compare(), which could be very slow. So I chose to cross-compile this. http://gerrit.cloudera.org:8080/#/c/10679/3/be/src/runtime/sorter-ir.cc@245 PS3, Line 245: void Sorter::TupleSorter::Swap(Tuple* left, Tuple* right, Tuple* swap_tuple, > Does this need to be cross-compiled ? It's in the main loop in Partition() and it's about only 3 instructions so why not? -- To view, visit http://gerrit.cloudera.org:8080/10679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaaf2b75c2f789002c42939865c018f728d29a113 Gerrit-Change-Number: 10679 Gerrit-PatchSet: 3 Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 12 Jun 2018 21:41:01 +0000 Gerrit-HasComments: Yes
