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 2016/10/12 03:56:05 UTC
[Impala-ASF-CR] Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4691
Change subject: Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
......................................................................
Reduce LLVM module's preparation time by lazily
creating the IRFunction::Type to LLVM::Function*
mappings.
Previously, when creating a LlvmCodeGen object, we
run an O(mn) algorithm to map the IRFunction::Type
to the actual LLVM::Function object in the module.
m is the size of IRFunction::Type enum and n is
the total number of functions in the module. This
is a waste of time if we only use few functions
from the module.
This change reduces the preparation time of a simple
query from 23ms to 10ms.
select count(*) from tpch100_parquet.lineitem where l_orderkey > 20;
Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
3 files changed, 184 insertions(+), 123 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/4691/1
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
......................................................................
Patch Set 1:
(5 comments)
I didn't realise that we spent so much time doing this. Change looks good, just could be cleaned up.
http://gerrit.cloudera.org:8080/#/c/4691/1/be/src/codegen/gen_ir_descriptions.py
File be/src/codegen/gen_ir_descriptions.py:
Line 104: ["HASH_CRC", "IrCrcHash"],
It's weird (but harmless, I guess) that we don't use C++ mangling for these.
http://gerrit.cloudera.org:8080/#/c/4691/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 206: for (int i = IRFunction::FN_START; i < IRFunction::FN_END; ++i) {
Maybe should be a new function like ValidateFunctionMappings()?
Line 209: if (init_codegen->module_->getFunction(fn_name) == NULL) {
We shouldn't hit this unless we messed up the build or the function names right? Could just be a DCHECK.
PS1, Line 407: StringRef
Should be able to construct the StringRef from the std::string directly without c_str(). It should be automatically converted I think if you just have getFunction(fn_name). We do this in LinkModule() - it works because of http://en.cppreference.com/w/cpp/language/converting_constructor
Line 668: StringRef fn_name(FN_MAPPINGS[ir_type].fn_name.c_str());
This conversion shouldn't be necessary either.
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
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
[Impala-ASF-CR] Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4691/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 209: StringRef fn_name(FN_MAPPINGS[i].fn_name);
> This StringRef conversion is redundant, can just inline FN_MAPPING[i].fn_na
Done. Not inlined but using a string const reference instead.
PS2, Line 403: StringRef
> This StringRef() is also redundant.
Done
Line 664: StringRef fn_name(FN_MAPPINGS[ir_type].fn_name);
> This StringRef() conversion is also redundant.
Done
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
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-4291: Reduce LLVM module's preparation time
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/4691
to look at the new patch set (#4).
Change subject: IMPALA-4291: Reduce LLVM module's preparation time
......................................................................
IMPALA-4291: Reduce LLVM module's preparation time
Previously, when creating a LlvmCodeGen object, we
run an O(mn) algorithm to map the IRFunction::Type
to the actual LLVM::Function object in the module.
m is the size of IRFunction::Type enum and n is
the total number of functions in the module. This
is a waste of time if we only use few functions
from the module.
This change reduces the preparation time of a simple
query from 23ms to 10ms.
select count(*) from tpch100_parquet.lineitem where l_orderkey > 20;
Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
2 files changed, 175 insertions(+), 123 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/4691/4
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Gerrit-PatchSet: 4
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>
[Impala-ASF-CR] IMPALA-4291: Reduce LLVM module's preparation time
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4291: Reduce LLVM module's preparation time
......................................................................
IMPALA-4291: Reduce LLVM module's preparation time
Previously, when creating a LlvmCodeGen object, we
run an O(mn) algorithm to map the IRFunction::Type
to the actual LLVM::Function object in the module.
m is the size of IRFunction::Type enum and n is
the total number of functions in the module. This
is a waste of time if we only use few functions
from the module.
This change reduces the preparation time of a simple
query from 23ms to 10ms.
select count(*) from tpch100_parquet.lineitem where l_orderkey > 20;
Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Reviewed-on: http://gerrit.cloudera.org:8080/4691
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
2 files changed, 175 insertions(+), 123 deletions(-)
Approvals:
Michael Ho: Looks good to me, approved
Internal Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4291: Reduce LLVM module's preparation time
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-4291: Reduce LLVM module's preparation time
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2).
Change subject: Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
......................................................................
Reduce LLVM module's preparation time by lazily
creating the IRFunction::Type to LLVM::Function*
mappings.
Previously, when creating a LlvmCodeGen object, we
run an O(mn) algorithm to map the IRFunction::Type
to the actual LLVM::Function object in the module.
m is the size of IRFunction::Type enum and n is
the total number of functions in the module. This
is a waste of time if we only use few functions
from the module.
This change reduces the preparation time of a simple
query from 23ms to 10ms.
select count(*) from tpch100_parquet.lineitem where l_orderkey > 20;
Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
2 files changed, 175 insertions(+), 123 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/4691/2
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
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] Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Gerrit-PatchSet: 3
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] Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4691/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 206: for (int i = IRFunction::FN_START; i < IRFunction::FN_END; ++i) {
> Maybe should be a new function like ValidateFunctionMappings()?
IMHO, this seems pretty readable and self-contained without factoring it into a separate function. Will add some comments though.
Line 209: if (init_codegen->module_->getFunction(fn_name) == NULL) {
> We shouldn't hit this unless we messed up the build or the function names r
Or if the in memory copy of the bitcode is corrupted somehow. That said, it can get corrupted any time since this check and GetFunction() will return NULL if the bitcode is corrupted somehow so may be okay to keep it as a DCHECK.
PS1, Line 407: StringRef
> Should be able to construct the StringRef from the std::string directly wit
Done
Line 668: StringRef fn_name(FN_MAPPINGS[ir_type].fn_name.c_str());
> This conversion shouldn't be necessary either.
Done
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Gerrit-PatchSet: 1
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-4291: Reduce LLVM module's preparation time
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4291: Reduce LLVM module's preparation time
......................................................................
Patch Set 4: Code-Review+2
Carry +2 forward.
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Gerrit-PatchSet: 4
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] Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#3).
Change subject: Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
......................................................................
Reduce LLVM module's preparation time by lazily
creating the IRFunction::Type to LLVM::Function*
mappings.
Previously, when creating a LlvmCodeGen object, we
run an O(mn) algorithm to map the IRFunction::Type
to the actual LLVM::Function object in the module.
m is the size of IRFunction::Type enum and n is
the total number of functions in the module. This
is a waste of time if we only use few functions
from the module.
This change reduces the preparation time of a simple
query from 23ms to 10ms.
select count(*) from tpch100_parquet.lineitem where l_orderkey > 20;
Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/llvm-codegen.cc
2 files changed, 175 insertions(+), 123 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/91/4691/3
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
Gerrit-PatchSet: 3
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>
[Impala-ASF-CR] Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Reduce LLVM module's preparation time by lazily creating the IRFunction::Type to LLVM::Function* mappings.
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4691/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 209: StringRef fn_name(FN_MAPPINGS[i].fn_name);
> Or if the in memory copy of the bitcode is corrupted somehow. That said, it
We're in real trouble if embedded data in the binary is corrupted anyway.
http://gerrit.cloudera.org:8080/#/c/4691/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 209: StringRef fn_name(FN_MAPPINGS[i].fn_name);
This StringRef conversion is redundant, can just inline FN_MAPPING[i].fn_name below.
PS2, Line 403: StringRef
This StringRef() is also redundant.
Line 664: StringRef fn_name(FN_MAPPINGS[ir_type].fn_name);
This StringRef() conversion is also redundant.
--
To view, visit http://gerrit.cloudera.org:8080/4691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I61ab9fa8cca5a0909bb716c3c62819da3e3b3041
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