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/03/04 06:52:09 UTC
[Impala-ASF-CR] IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6263
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
To support sharing generated code across fragment instances,
no fragment instance specific states should be baked into the
IR. All cases were addressed previously except for the old hash
tables used for legacy agg/join. This change fixes the old hash
tables to not bake its fields in the generated IR functions. Similar
to previous patches, some cross-compiled thin wrappers are introduced
to access the fields of interest from an OldHashTable object.
Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/exec/CMakeLists.txt
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
A be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/old-hash-table.h
9 files changed, 171 insertions(+), 68 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/6263/1
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
Patch Set 3: Code-Review+2
Carry Tim's +2
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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] IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
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/6263
to look at the new patch set (#3).
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
To support sharing generated code across fragment instances,
no fragment instance specific states should be baked into the
IR. All cases were addressed previously except for the old hash
tables used for legacy agg/join. This change fixes the old hash
tables to not bake its fields in the generated IR functions. Similar
to previous patches, some cross-compiled thin wrappers are introduced
to access the fields of interest from an OldHashTable object.
Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/CMakeLists.txt
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
A be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/old-hash-table.h
11 files changed, 184 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/6263/3
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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] IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/6263/1//COMMIT_MSG
Commit Message:
Line 16:
I did a grep for CastPtrToLlvmPtr() to make sure there were no other instances of this pattern. I noticed in scalar-fn-call.cc there's some dead code that uses this pattern - maybe we should just remove it while we're working on a related patch.
http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/hash-table-ir.cc
File be/src/exec/hash-table-ir.cc:
Line 26: ExprContext* const* HashTableCtx::GetBuildExprCtxs() const { return build_expr_ctxs_.data(); }
Long lines
http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
Line 736: Value* ctx_vector = builder.CreateCall(get_expr_ctx_fn, this_ptr, "ctx_vector");
Comment that this is pointer to the first ExprContext* pointer?
Line 1099: Value* ctx_vector = builder.CreateCall(get_expr_ctx_fn, this_ptr, "ctx_vector");
Maybe comment that this is a pointer to the first ExprContext*?
Line 1121: // Load ExprContext* from 'build_expr_ctxs_'.
No longer accurate
http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/old-hash-table.cc
File be/src/exec/old-hash-table.cc:
Line 651: Function* get_build_expr_ctx_fn =
What do you think about adding a CallFunction() helper to LlvmCodegen to make the GetFunction()/CreateCall() pattern more concise.
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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] IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6263/1//COMMIT_MSG
Commit Message:
Line 16:
> I looked at that instance at ScalarFnCall(). It t should be fine for MT as
I looked and it seemed like the fallback would never be used since the GetCodegendComputeFn() should never return OK + NULL. Not a big deal anyway.
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
I feel pretty confident +2ing - we have good test coverage of the in-memory joins and this patch is only changing the internal implementation of deprecated code.
I assume you did a test run using this code path but if not it would be good to do before merging.
http://gerrit.cloudera.org:8080/#/c/6263/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 1458: // Load &build_expr_ctxs_[0]
Stray comment.
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2).
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
To support sharing generated code across fragment instances,
no fragment instance specific states should be baked into the
IR. All cases were addressed previously except for the old hash
tables used for legacy agg/join. This change fixes the old hash
tables to not bake its fields in the generated IR functions. Similar
to previous patches, some cross-compiled thin wrappers are introduced
to access the fields of interest from an OldHashTable object.
Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/CMakeLists.txt
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
A be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/old-hash-table.h
11 files changed, 185 insertions(+), 80 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/6263/2
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
Patch Set 2:
(2 comments)
Yes, I did a private run with legacy join and agg before sending this out for review. Will do another run again before merging.
http://gerrit.cloudera.org:8080/#/c/6263/1//COMMIT_MSG
Commit Message:
Line 16:
> I looked and it seemed like the fallback would never be used since the GetC
Didn't mean to rathole on this. Apparently, we didn't check for the return status of GetCodegendComputeFn() in that case. Anyhow, it seems nice to be able to interpret the child expression if codegen failed for it.
http://gerrit.cloudera.org:8080/#/c/6263/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
Line 1458: // Load &build_expr_ctxs_[0]
> Stray comment.
Done
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
Patch Set 1:
(1 comment)
Forgot to mention - the patch looks good aside from the minor things. If we were planning to support OldHashTable longer-term we could clear up that code a bit but this looks fine as a fix for code we're working on deprecating.
http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/old-hash-table.cc
File be/src/exec/old-hash-table.cc:
Line 325: // TODO: refactor this to somewhere else? This is not hash table specific
Remove TODO if you're not going to do this in this patch?
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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] IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
Patch Set 3:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/371/
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/6263/1//COMMIT_MSG
Commit Message:
Line 16:
> I did a grep for CastPtrToLlvmPtr() to make sure there were no other instan
I looked at that instance at ScalarFnCall(). It t should be fine for MT as Expr objects are shared across fragment instances. My understanding of that path is that it's a fall back to the interpretation path if codegen fails for a child expression so they aren't dead code but I think there are other ways of generating those code without cross-compiling the static stub functions in expr-ir.cc
http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/hash-table-ir.cc
File be/src/exec/hash-table-ir.cc:
Line 26: ExprContext* const* HashTableCtx::GetBuildExprCtxs() const {
> Long lines
Done
http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:
Line 736:
> Comment that this is pointer to the first ExprContext* pointer?
Done
Line 1099: BasicBlock* null_block = BasicBlock::Create(context, "null", *fn);
> Maybe comment that this is a pointer to the first ExprContext*?
Done
Line 1121: CodegenAnyVal result = CodegenAnyVal::CreateCallWrapped(codegen, &builder,
> No longer accurate
It's still accurate in a way but I rephrased it to:
expr_ctx = ctx_vector[i];
http://gerrit.cloudera.org:8080/#/c/6263/1/be/src/exec/old-hash-table.cc
File be/src/exec/old-hash-table.cc:
Line 325: llvm_loc = builder.CreatePointerCast(llvm_loc,
> Remove TODO if you're not going to do this in this patch?
Done. old-hash-table's day is numbered anyway.
Line 651:
> What do you think about adding a CallFunction() helper to LlvmCodegen to ma
Done
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
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-4008: Don't bake fields into generated IR functions of OldHashTable
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
......................................................................
IMPALA-4008: Don't bake fields into generated IR functions of OldHashTable
To support sharing generated code across fragment instances,
no fragment instance specific states should be baked into the
IR. All cases were addressed previously except for the old hash
tables used for legacy agg/join. This change fixes the old hash
tables to not bake its fields in the generated IR functions. Similar
to previous patches, some cross-compiled thin wrappers are introduced
to access the fields of interest from an OldHashTable object.
Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
Reviewed-on: http://gerrit.cloudera.org:8080/6263
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/CMakeLists.txt
M be/src/exec/hash-table-ir.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
A be/src/exec/old-hash-table-ir.cc
M be/src/exec/old-hash-table.cc
M be/src/exec/old-hash-table.h
11 files changed, 184 insertions(+), 80 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Michael Ho: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6263
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I75500827dff56b1fa9e5296e8e8d8667ab54aef8
Gerrit-PatchSet: 4
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>