You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2017/06/09 20:00:42 UTC

[Impala-ASF-CR] IMPALA-5479: Propagate the argument type in RawValue::Compare() 

Michael Ho has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7140

Change subject: IMPALA-5479: Propagate the argument type in RawValue::Compare()
......................................................................

IMPALA-5479: Propagate the argument type in RawValue::Compare()

CodegenAnyVal::Compare() generates code which calls the cross
compiled version of RawValue::Compare() without propagating
the type information into RawValue::Compare(). As a result,
the generated code of RawValue::Compare() is not any more
efficient than the interpreted version as we still have
the big switch statement in it.

This change creates a global constant for the argument 'type'
passed to RawValue::Compare(). By inlining the call to
RawValue::Compare(), LLVM was able to constant propagate
the type and eliminate the dead code for non-target types.

With this change, a query with top-n improves by 12% on average:

select l_orderkey, l_partkey, l_suppkey
from tpch50_parquet.lineitem
order by l_orderkey, l_partkey, l_suppkey
limit 10000;

4.49s -> 3.95s

This change also adds the ALWAYS_INLINE attribute to
RuntimeFilter::Eval() as it's needed to propagate the type
after a recent change to not put ALWAYS_INLINE attribute
on all cross-compiled functions.

Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
---
M be/src/codegen/codegen-anyval.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.h
M be/src/runtime/runtime-filter-ir.cc
M be/src/runtime/runtime-filter.h
5 files changed, 14 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/7140/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-5479: Propagate the argument type in RawValue::Compare()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5479: Propagate the argument type in RawValue::Compare()
......................................................................


Patch Set 2: Verified+1

-- 
To view, visit http://gerrit.cloudera.org:8080/7140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5479: Propagate the argument type in RawValue::Compare()

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-5479: Propagate the argument type in RawValue::Compare()
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

Line 702
> Its a little weird that it can't infer that this is already constant. The n
We were using a stack variable. LLVM is hesitant to propagate that as a constant.


http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/runtime/raw-value-ir.cc
File be/src/runtime/raw-value-ir.cc:

PS1, Line 29: IR_ALWAYS_INLINE
> We don't need the attribute in both declaration and definition do we?
Agree that it's not needed but it seems clearer (for documentation purpose) to have it at the definition too. It seems to be the prevalent pattern in our cross-compiled code now.


http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/runtime/runtime-filter-ir.cc
File be/src/runtime/runtime-filter-ir.cc:

PS1, Line 24: IR_ALWAYS_INLINE
> We don't need the attribute in both declaration and definition do we?
Agree that it's not needed but it seems clearer (for documentation purpose) to have it at the definition too. It seems to be the prevalent pattern in our cross-compiled code now. Do you feel strongly that we should clean them up ?


http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/runtime/runtime-filter.h
File be/src/runtime/runtime-filter.h:

Line 63:   /// Inlined in IR so that the constant 'col_type' can be propagated.
> Maybe something like "Inlined in IR so that the constnt 'col_type' can be p
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5479: Propagate the argument type in RawValue::Compare()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5479: Propagate the argument type in RawValue::Compare()
......................................................................


IMPALA-5479: Propagate the argument type in RawValue::Compare()

CodegenAnyVal::Compare() generates code which calls the cross
compiled version of RawValue::Compare() without propagating
the type information into RawValue::Compare(). As a result,
the generated code of RawValue::Compare() is not any more
efficient than the interpreted version as we still have
the big switch statement in it.

This change creates a global constant for the argument 'type'
passed to RawValue::Compare(). By inlining the call to
RawValue::Compare(), LLVM was able to constant propagate
the type and eliminate the dead code for non-target types.

With this change, a query with top-n improves by 12% on average:

select l_orderkey, l_partkey, l_suppkey
from tpch50_parquet.lineitem
order by l_orderkey, l_partkey, l_suppkey
limit 10000;

4.49s -> 3.95s

This change also adds the ALWAYS_INLINE attribute to
RuntimeFilter::Eval() as it's needed to propagate the type
after a recent change to not put ALWAYS_INLINE attribute
on all cross-compiled functions.

Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
Reviewed-on: http://gerrit.cloudera.org:8080/7140
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/codegen-anyval.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.h
M be/src/runtime/runtime-filter-ir.cc
M be/src/runtime/runtime-filter.h
5 files changed, 21 insertions(+), 11 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/7140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5479: Propagate the argument type in RawValue::Compare()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5479: Propagate the argument type in RawValue::Compare()
......................................................................


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/718/

-- 
To view, visit http://gerrit.cloudera.org:8080/7140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5479: Propagate the argument type in RawValue::Compare()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5479: Propagate the argument type in RawValue::Compare()
......................................................................


Patch Set 2: Code-Review+2

The IR_ALWAYS_INLINE seems fine as-is

-- 
To view, visit http://gerrit.cloudera.org:8080/7140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5479: Propagate the argument type in RawValue::Compare()

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/7140

to look at the new patch set (#2).

Change subject: IMPALA-5479: Propagate the argument type in RawValue::Compare()
......................................................................

IMPALA-5479: Propagate the argument type in RawValue::Compare()

CodegenAnyVal::Compare() generates code which calls the cross
compiled version of RawValue::Compare() without propagating
the type information into RawValue::Compare(). As a result,
the generated code of RawValue::Compare() is not any more
efficient than the interpreted version as we still have
the big switch statement in it.

This change creates a global constant for the argument 'type'
passed to RawValue::Compare(). By inlining the call to
RawValue::Compare(), LLVM was able to constant propagate
the type and eliminate the dead code for non-target types.

With this change, a query with top-n improves by 12% on average:

select l_orderkey, l_partkey, l_suppkey
from tpch50_parquet.lineitem
order by l_orderkey, l_partkey, l_suppkey
limit 10000;

4.49s -> 3.95s

This change also adds the ALWAYS_INLINE attribute to
RuntimeFilter::Eval() as it's needed to propagate the type
after a recent change to not put ALWAYS_INLINE attribute
on all cross-compiled functions.

Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
---
M be/src/codegen/codegen-anyval.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.h
M be/src/runtime/runtime-filter-ir.cc
M be/src/runtime/runtime-filter.h
5 files changed, 21 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/7140/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-5479: Propagate the argument type in RawValue::Compare()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5479: Propagate the argument type in RawValue::Compare()
......................................................................


Patch Set 1: Code-Review+2

(4 comments)

Nice!

http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

Line 702
Its a little weird that it can't infer that this is already constant. The new code seems better anyway.


http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/runtime/raw-value-ir.cc
File be/src/runtime/raw-value-ir.cc:

PS1, Line 29: IR_ALWAYS_INLINE
We don't need the attribute in both declaration and definition do we?


http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/runtime/runtime-filter-ir.cc
File be/src/runtime/runtime-filter-ir.cc:

PS1, Line 24: IR_ALWAYS_INLINE
We don't need the attribute in both declaration and definition do we?


http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/runtime/runtime-filter.h
File be/src/runtime/runtime-filter.h:

Line 63:   bool IR_ALWAYS_INLINE Eval(void* val, const ColumnType& col_type) const noexcept;
Maybe something like "Inlined in IR so that the constnt 'col_type' can be propagated"


-- 
To view, visit http://gerrit.cloudera.org:8080/7140
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes