Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Impala 2.7.0
    • Fix Version/s: Impala 2.8.0
    • Component/s: Backend
    • Labels:

      Description

      The IR generated for the TopN node is pretty suboptimal. We codegen the outer Insert loop, and we codegen the comparator function, but we don't convert the indirect call to a function pointer comparator into a direct inlinable function call. The below C++ code is just cross-compile wholesale without removing the branch or function pointer, and also bringing in a huge amount of unnecessary IR for the interpreted Compare() path.

        bool ALWAYS_INLINE Less(const TupleRow* lhs, const TupleRow* rhs) const {
          int result = codegend_compare_fn_ == NULL ? Compare(lhs, rhs) :
              (*codegend_compare_fn_)(&key_expr_ctxs_lhs_[0], &key_expr_ctxs_rhs_[0], lhs, rhs);
          if (result < 0) return true;
          return false;
        }
      

        Issue Links

          Activity

          Hide
          tarmstrong Tim Armstrong added a comment -

          IMPALA-3815: clean up cross-compiled comparator

          This avoids including a big block of interpreted code in the
          cross-compiled IR and will make it easier to inline the Compare()
          function into other codegened functions in a later change.

          Perf:
          Ran top-n targeted perf, didn't see any significant change.

          Change-Id: I058917da2c13ba41d6ff7fefbb761606344312ab
          Reviewed-on: http://gerrit.cloudera.org:8080/4307
          Reviewed-by: Dan Hecht <dhecht@cloudera.com>
          Tested-by: Internal Jenkins

          Show
          tarmstrong Tim Armstrong added a comment - IMPALA-3815 : clean up cross-compiled comparator This avoids including a big block of interpreted code in the cross-compiled IR and will make it easier to inline the Compare() function into other codegened functions in a later change. Perf: Ran top-n targeted perf, didn't see any significant change. Change-Id: I058917da2c13ba41d6ff7fefbb761606344312ab Reviewed-on: http://gerrit.cloudera.org:8080/4307 Reviewed-by: Dan Hecht <dhecht@cloudera.com> Tested-by: Internal Jenkins
          Hide
          tarmstrong Tim Armstrong added a comment -

          Sounds good to me, just wanted to be sure that you didn't have a patch waiting in the wings

          Show
          tarmstrong Tim Armstrong added a comment - Sounds good to me, just wanted to be sure that you didn't have a patch waiting in the wings
          Hide
          yonghyun_impala_8905 Yonghyun Hwang added a comment -

          I was pulled out for other works and couldn't have a time to try the fix. Please take it and let me follow up to have a good understanding on this.

          Show
          yonghyun_impala_8905 Yonghyun Hwang added a comment - I was pulled out for other works and couldn't have a time to try the fix. Please take it and let me follow up to have a good understanding on this.
          Hide
          tarmstrong Tim Armstrong added a comment -

          Did you start looking at this? I was experimenting with some other codegen changes and I may end up fixing this incidentally.

          Show
          tarmstrong Tim Armstrong added a comment - Did you start looking at this? I was experimenting with some other codegen changes and I may end up fixing this incidentally.

            People

            • Assignee:
              tarmstrong Tim Armstrong
              Reporter:
              tarmstrong Tim Armstrong
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development