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/04/17 20:36:08 UTC
[Impala-ASF-CR] IMPALA-5129: Don't bake MemPool* into IR
Michael Ho has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6657
Change subject: IMPALA-5129: Don't bake MemPool* into IR
......................................................................
IMPALA-5129: Don't bake MemPool* into IR
Tuple::CodegenMaterializeExprs() currently bakes the MemPool*
provided by its caller into the generated IR. The MemPool*
usually belongs to some exec nodes which owns the codegend
function and it's used for allocating string buffer. With
multi-threading, IR needs to be shared across multiple fragment
instances so IR can no longer contain pointers not shared
across fragment instances.
This change fixes the problem above by using the MemPool*
argument passed to the IR function. This also cleans up
UnionNode by removing the field tuple_pool_ from it and
the logic for transferring buffer from tuple_pool_ to the
MemPool of the row batch.
Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/union-node-ir.cc
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/runtime/tuple.cc
M be/src/runtime/tuple.h
11 files changed, 87 insertions(+), 111 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6657/1
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 5:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/508/
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 3: Code-Review+1
Carry Tim's and Tara's +1 forward.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
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: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
IMPALA-5192: Don't bake MemPool* into IR
Tuple::CodegenMaterializeExprs() currently bakes the MemPool*
provided by its caller into the generated IR. The MemPool*
usually belongs to some exec nodes which owns the codegend
function and it's used for allocating string buffer. With
multi-threading, IR needs to be shared across multiple fragment
instances so IR can no longer contain pointers not shared
across fragment instances.
This change fixes the problem above by using the MemPool*
argument passed to the IR function. This also cleans up
UnionNode by removing the field tuple_pool_ from it and
the logic for transferring buffer from tuple_pool_ to the
MemPool of the row batch.
Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Reviewed-on: http://gerrit.cloudera.org:8080/6657
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/union-node-ir.cc
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/runtime/tuple.cc
M be/src/runtime/tuple.h
11 files changed, 107 insertions(+), 121 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Michael Ho: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 2:
(4 comments)
Thanks for the review.
http://gerrit.cloudera.org:8080/#/c/6657/2/be/src/codegen/codegen-anyval.h
File be/src/codegen/codegen-anyval.h:
Line 200: /// If 'pool_val' is non-NULL, var-len data will be copied into 'pool_val'.
> We should explain that pool_val needs to be a MemPool*
Done
Line 208: /// If 'pool_val' is non-NULL, var-len data will be copied into 'pool_val'.
> Same here
Done
Line 224: llvm::Value* pool_val, llvm::BasicBlock* insert_before = nullptr);
> And here
Done
http://gerrit.cloudera.org:8080/#/c/6657/2/be/src/runtime/tuple.h
File be/src/runtime/tuple.h:
Line 165: /// 'pool' may be NULL, in which case no pool-related code is generated. Otherwise
> Comment needs updating.
Done
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
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: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2).
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
IMPALA-5192: Don't bake MemPool* into IR
Tuple::CodegenMaterializeExprs() currently bakes the MemPool*
provided by its caller into the generated IR. The MemPool*
usually belongs to some exec nodes which owns the codegend
function and it's used for allocating string buffer. With
multi-threading, IR needs to be shared across multiple fragment
instances so IR can no longer contain pointers not shared
across fragment instances.
This change fixes the problem above by using the MemPool*
argument passed to the IR function. This also cleans up
UnionNode by removing the field tuple_pool_ from it and
the logic for transferring buffer from tuple_pool_ to the
MemPool of the row batch.
Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/union-node-ir.cc
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/runtime/tuple.cc
M be/src/runtime/tuple.h
11 files changed, 87 insertions(+), 111 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6657/2
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 2: Code-Review+1
Looks good to me too. Agree with Tim that comments need to be updated.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 4: Code-Review+1
Carry +1 forward.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 2: Code-Review+1
(4 comments)
This looks good to me aside from some comments that need updating.
http://gerrit.cloudera.org:8080/#/c/6657/2/be/src/codegen/codegen-anyval.h
File be/src/codegen/codegen-anyval.h:
Line 200: /// If 'pool_val' is non-NULL, var-len data will be copied into 'pool_val'.
We should explain that pool_val needs to be a MemPool*
Line 208: /// If 'pool_val' is non-NULL, var-len data will be copied into 'pool_val'.
Same here
Line 224: llvm::Value* pool_val, llvm::BasicBlock* insert_before = nullptr);
And here
http://gerrit.cloudera.org:8080/#/c/6657/2/be/src/runtime/tuple.h
File be/src/runtime/tuple.h:
Line 165: /// 'pool' may be NULL, in which case no pool-related code is generated. Otherwise
Comment needs updating.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/6657/3/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:
PS3, Line 528: CodegenAllocate
okay to ignore, but I think this would be clearer if called CodegenMemPoolAllocate().
http://gerrit.cloudera.org:8080/#/c/6657/3/be/src/runtime/tuple.cc
File be/src/runtime/tuple.cc:
PS3, Line 344: // Value* non_null_string_values_arg = args[5]; // unused
: // Value* total_string_lengths_arg = args[6]; // unused
: // Value* num_non_null_string_values_arg = args[7]; // unused
what does this mean? that these out-parameters can't be used with codegen? do we document that somewhere?
http://gerrit.cloudera.org:8080/#/c/6657/3/be/src/runtime/tuple.h
File be/src/runtime/tuple.h:
PS3, Line 121: .
it would probably be good to mention the equivalent thing about codegen -- that it has to match up with the use_mem_pool argument to CodegenMaterializeExprs()
PS3, Line 167: There are two different MaterializeExprs symbols to differentiate between
: /// these cases when we replace the function calls during codegen.
what is this referring to exactly? we generate two versions of the same function? i don't see where that happens.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 5: Code-Review+2
Rebase. Carry +2.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6657/3/be/src/runtime/tuple.h
File be/src/runtime/tuple.h:
PS3, Line 167: There are two different MaterializeExprs symbols to differentiate between
: /// these cases when we replace the function calls during codegen.
> I see. But why do we need all that in the first place? All of this code get
Yes, that's another way of doing it.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 3:
(4 comments)
Will post new patch after https://gerrit.cloudera.org/#/c/6459/ is merged.
http://gerrit.cloudera.org:8080/#/c/6657/3/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:
PS3, Line 528: CodegenAllocate
> okay to ignore, but I think this would be clearer if called CodegenMemPoolA
Done
http://gerrit.cloudera.org:8080/#/c/6657/3/be/src/runtime/tuple.cc
File be/src/runtime/tuple.cc:
PS3, Line 344: // Value* non_null_string_values_arg = args[5]; // unused
: // Value* total_string_lengths_arg = args[6]; // unused
: // Value* num_non_null_string_values_arg = args[7]; // unused
> what does this mean? that these out-parameters can't be used with codegen?
This function makes an assumption that 'collect_string_vals' is false so these arguments are not used. Added some comments for it.
http://gerrit.cloudera.org:8080/#/c/6657/3/be/src/runtime/tuple.h
File be/src/runtime/tuple.h:
PS3, Line 121: .
> it would probably be good to mention the equivalent thing about codegen --
Done
PS3, Line 167: There are two different MaterializeExprs symbols to differentiate between
: /// these cases when we replace the function calls during codegen.
> what is this referring to exactly? we generate two versions of the same fun
Please see Tuple::MATERIALIZE_EXPRS_SYMBOL and Tuple::MATERIALIZE_EXPRS_NULL_POOL_SYMBOL. Comments updated to refer to the comment of MaterializeExprs() about the two different symbols.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6657/4/be/src/runtime/tuple.cc
File be/src/runtime/tuple.cc:
Line 299: }
why is this a return status now? we don't want to have callsites do this (and we know that they don't), right?
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Taras Bobrovytsky, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6657
to look at the new patch set (#4).
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
IMPALA-5192: Don't bake MemPool* into IR
Tuple::CodegenMaterializeExprs() currently bakes the MemPool*
provided by its caller into the generated IR. The MemPool*
usually belongs to some exec nodes which owns the codegend
function and it's used for allocating string buffer. With
multi-threading, IR needs to be shared across multiple fragment
instances so IR can no longer contain pointers not shared
across fragment instances.
This change fixes the problem above by using the MemPool*
argument passed to the IR function. This also cleans up
UnionNode by removing the field tuple_pool_ from it and
the logic for transferring buffer from tuple_pool_ to the
MemPool of the row batch.
Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/union-node-ir.cc
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/runtime/tuple.cc
M be/src/runtime/tuple.h
11 files changed, 107 insertions(+), 121 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6657/4
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6657/4/be/src/runtime/tuple.cc
File be/src/runtime/tuple.cc:
Line 299: }
> why is this a return status now? we don't want to have callsites do this (a
To be consistent with other codegen functions which returns an error status for unsupported data type.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Taras Bobrovytsky, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6657
to look at the new patch set (#3).
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
IMPALA-5192: Don't bake MemPool* into IR
Tuple::CodegenMaterializeExprs() currently bakes the MemPool*
provided by its caller into the generated IR. The MemPool*
usually belongs to some exec nodes which owns the codegend
function and it's used for allocating string buffer. With
multi-threading, IR needs to be shared across multiple fragment
instances so IR can no longer contain pointers not shared
across fragment instances.
This change fixes the problem above by using the MemPool*
argument passed to the IR function. This also cleans up
UnionNode by removing the field tuple_pool_ from it and
the logic for transferring buffer from tuple_pool_ to the
MemPool of the row batch.
Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/topn-node-ir.cc
M be/src/exec/topn-node.cc
M be/src/exec/union-node-ir.cc
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/runtime/tuple.cc
M be/src/runtime/tuple.h
11 files changed, 95 insertions(+), 116 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/57/6657/3
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5192: Don't bake MemPool* into IR
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-5192: Don't bake MemPool* into IR
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6657/3/be/src/runtime/tuple.h
File be/src/runtime/tuple.h:
PS3, Line 167: There are two different MaterializeExprs symbols to differentiate between
: /// these cases when we replace the function calls during codegen.
> Please see Tuple::MATERIALIZE_EXPRS_SYMBOL and Tuple::MATERIALIZE_EXPRS_NUL
I see. But why do we need all that in the first place? All of this code gets inlined, right? So, it seems that we could just emit the null check against pool into the codegn and then let inlining and constant propagation take care of optimizing it out of the IR. Anyway, we can look at that as a follow on cleanup later.
--
To view, visit http://gerrit.cloudera.org:8080/6657
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I09d620e48032351ab9805825a4afb6536bed2302
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes