You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2020/03/20 01:18:35 UTC
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15408
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
IMPALA-4080: Codegen once per fragment
This is the final patch:
- Removes all temporary changes that called codegen on plan nodes and
data sink through the exec node.
- Plan node creation and codegen is done in a separate thread for every
fragment.
- Introduces FragmentState class that basically contains the plan tree,
data sink config and the codegen state for every fragment.
- Every FragmentState object has its own runtime profile that helps to
map out every codegen object's profile to its fragment id.
Testing:
Ran exhaustive tests successfully except for 2 tests.
TODO:
- Fix the failed tests namely:
test_under_statement_expression_limit
test_exchange_mem_usage_scaling
- Fix all TODOs, which are mostly adding comments
Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/blocking-join-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-avro-scanner.h
M be/src/exec/hdfs-columnar-scanner.cc
M be/src/exec/hdfs-columnar-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/join-builder.cc
M be/src/exec/join-builder.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.cc
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/exec/select-node.cc
M be/src/exec/select-node.h
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/subplan-node.cc
M be/src/exec/subplan-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/exprs/agg-fn.cc
M be/src/exprs/agg-fn.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/coordinator.cc
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
A be/src/runtime/fragment-state.cc
A be/src/runtime/fragment-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf.cc
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
M common/protobuf/control_service.proto
101 files changed, 877 insertions(+), 593 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15408/7
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5684/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 12
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Apr 2020 17:37:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15408/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15408/8//COMMIT_MSG@26
PS8, Line 26: TODO:
This can be updated right?
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Mar 2020 17:46:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 10:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/15408/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15408/7//COMMIT_MSG@18
PS7, Line 18: parent fragment does the actual codegen work and the rest of the
> Maybe include an example of how the codegen in the profile looks after the
Done
http://gerrit.cloudera.org:8080/#/c/15408/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15408/8//COMMIT_MSG@26
PS8, Line 26: An example of how the codegen in the profile looks:
> This can be updated right?
Done
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.h
File be/src/runtime/fragment-state.h:
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.h@42
PS8, Line 42: QueryState* state, std::unordered_map<TFragmentIdx, FragmentState*>& fragment_map);
> Missed this TODO
Done
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.h@168
PS8, Line 168: Status codegen_status_;
> Same here
Done
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.cc
File be/src/runtime/fragment-state.cc:
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.cc@84
PS8, Line 84: codegen_status_.AddDetail(error_ctx);
> It would be good to do this outside of the critical section so that we don'
As we discussed offline, keeping this unchanged to avoid fragment instance threads racing to set the error status in queryState. Also documented the lock ordering over codegen_lock_
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/query-state.cc@548
PS8, Line 548: if (!HasErrorStatus()) {
> nit: extra braces for lock scope not needed
Done
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Mar 2020 20:16:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
Patch Set 9:
rebased
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 9
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Mar 2020 02:18:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
Patch Set 7:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/15408/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15408/7//COMMIT_MSG@18
PS7, Line 18:
Maybe include an example of how the codegen in the profile looks after the changes?
http://gerrit.cloudera.org:8080/#/c/15408/7//COMMIT_MSG@21
PS7, Line 21:
We'll need to do some perf work for the non-mt_dop code to make sure there are no regressions.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/codegen/llvm-codegen-test.cc
File be/src/codegen/llvm-codegen-test.cc:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/codegen/llvm-codegen-test.cc@54
PS7, Line 54: QueryState* qs = runtime_state_->query_state();
Maybe this could be done in CreateQueryState()? This generic setup stuff is more reuseable if it's there.
I think this is probably fine though.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/exec/exec-node.cc@150
PS7, Line 150: codegen_status_msgs_.push_back(std::move(FragmentState::GenerateCodegenMsg(
I'm not sure that the move does anything if GenerateCodegenMsg() is returned by value. I think emplace_back() might avoid a copy though.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/exec/exec-node.cc@227
PS7, Line 227: NULL
switch to nullptr while you're changing it in this file?
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/exec/exec-node.cc@228
PS7, Line 228: for (int i = 0; i < children_.size(); ++i) {
Maybe change to range-based for?
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h
File be/src/runtime/fragment-state.h:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h@19
PS7, Line 19: #ifndef IMPALA_RUNTIME_FRAGMENT_STATE_H
Use a #pragma once instead of the old-style included guards
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h@26
PS7, Line 26: #include "runtime/runtime-filter-bank.h"
This doesn't look like it's needed.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h@32
PS7, Line 32: class RuntimeFilterBank;
This doesn't look like it's needed.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h@56
PS7, Line 56: const TPlanFragmentInstanceCtx*
Might be better to use references, since they should be non-nullable. Not a big deal though
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.cc
File be/src/runtime/fragment-state.cc:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.cc@97
PS7, Line 97: query_state->obj_pool(), Substitute("Fragment-$0", fragment_idx()));
Should use the same fragment ID format (F01, etc) as elsewhere in the profile/plan.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.h@387
PS7, Line 387: /// TODO: add description
This
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@267
PS7, Line 267: // TODO: instance_ctxs contains all instances among all fragments, I need to run this
Does this need to be done in this patchset?
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@608
PS7, Line 608:
nit: probably don't need this vertical whitespace.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@612
PS7, Line 612: string thread_name = Substitute("$0 (fragment idx:$1)",
It would be be better to use the usual fragment ID (F01, etc), since that's the identifier we generally use.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@621
PS7, Line 621: for (unique_ptr<Thread>& thread : codegen_threads) {
This has the potential to change the timing of some queries significantly, since the fragments will not start executing until the last codegen fragment
This may be a good thing for predictability, but is a bad thing for parallelism, since it may delay starting up small scan fragments, etc.
I suspect it may add enough risk of regression that we want to avoid it (at least for non-mt_dop queries). I think we could approximate the old behaviour if each codegen thread started up the instances for that fragment once it finished codegen (I guess then it becomes a fragment start thread).
Also, have you thought about how slow codegen could be debugged in the profile now? I think it's good that the codegen sub-profiles are more prominent for each backend. But tracing back from a single fragment profile, I guess we'd have to look at the "Open started" in the timeline to see how much codegen delayed the execution startup.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@622
PS7, Line 622: thread->Join();
I wonder about whether using a thread pool would be a good idea here, just to reduce thread startup overhead and context switching under concurrency.
I think there would be some concerns here about fairness between queries and reflecting queuing delays in the profile, so *probably* not a good idea to tackle right now.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@805
PS7, Line 805: // TODO: maybe just return the FragmentState and use that instead
Comment probably needs cleanup?
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf-internal.h
File be/src/udf/udf-internal.h:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf-internal.h@174
PS7, Line 174: static int GetConstFnAttr(bool decimalV2,
Maybe pass in the query options struct?
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf.cc
File be/src/udf/udf.cc:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf.cc@122
PS7, Line 122: // TODO: bik: will have to add a fake class above if I want to include any of the impala
Does this need addressing?
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Mar 2020 17:59:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
Patch Set 8:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5616/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Mar 2020 02:08:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
Patch Set 9:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5618/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 9
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Mar 2020 02:59:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 11
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Mar 2020 21:05:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 10:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5669/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Mar 2020 21:00:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 12: Code-Review+2
fixed clang-tidy error
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 12
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Apr 2020 16:57:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 13: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 13
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Apr 2020 18:23:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15408
to look at the new patch set (#8).
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
IMPALA-4080: Codegen once per fragment
This is the final patch:
- Removes all temporary changes that called codegen on plan nodes and
data sink through the exec node.
- Introduces FragmentState class that basically contains the plan tree,
data sink config and the codegen state for every fragment.
- Every FragmentState object has its own runtime profile that helps to
map out every codegen object's profile to its fragment id.
- Plan nodes are generated before fragment instance threads are spawned.
- The first fragment instance thread to call codegen on its parent
parent fragment does the actual codegen work and the rest of the
instance threads wait till codegen is complete.
- This very closely mimics the timing of the state machines both for
query state and the fragment instance states.
Testing:
Ran exhaustive tests successfully except for 2 tests.
TODO:
- Fix the failed tests namely:
test_under_statement_expression_limit - need to confirm if the
timing change fixed this too.
test_exchange_mem_usage_scaling - fixing the timing of the query
pipeline fixed this.
- Fix all TODOs, which are mostly adding comments
Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/blocking-join-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-avro-scanner.h
M be/src/exec/hdfs-columnar-scanner.cc
M be/src/exec/hdfs-columnar-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/join-builder.cc
M be/src/exec/join-builder.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.cc
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/exec/select-node.cc
M be/src/exec/select-node.h
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/subplan-node.cc
M be/src/exec/subplan-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/exprs/agg-fn.cc
M be/src/exprs/agg-fn.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
A be/src/runtime/fragment-state.cc
A be/src/runtime/fragment-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf.cc
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
100 files changed, 914 insertions(+), 625 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15408/8
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 11: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5592/
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 11
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Apr 2020 01:36:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
Patch Set 7:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/5537/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Mar 2020 02:04:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
Patch Set 8:
(5 comments)
Looking good! I have only minor comments. It looks like there are quite a few TODOs for comments, so I'm assuming you're going to get back to those.
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.h
File be/src/runtime/fragment-state.h:
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.h@42
PS8, Line 42: // TODO: add desc
Missed this TODO
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.h@168
PS8, Line 168: /// TODO: add description
Same here
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.cc
File be/src/runtime/fragment-state.cc:
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/fragment-state.cc@84
PS8, Line 84: query_state_->ErrorDuringFragmentCodegen(codegen_status_);
It would be good to do this outside of the critical section so that we don't need to worry about lock ordering between codegen_lock_ and QueryState::status_lock_
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/15408/8/be/src/runtime/query-state.cc@548
PS8, Line 548: {
nit: extra braces for lock scope not needed
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf-internal.h
File be/src/udf/udf-internal.h:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf-internal.h@174
PS7, Line 174: static int GetConstFnAttr(bool uses_decimal_v2,
> I decided to just pass this bool here (just renamed it to uses_decimal_v2)
ok sounds fine
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Mar 2020 17:45:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
Patch Set 8:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/exec/exec-node.cc@150
PS7, Line 150: codegen_status_msgs_.emplace_back(FragmentState::GenerateCodegenMsg(
> I'm not sure that the move does anything if GenerateCodegenMsg() is returne
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/exec/exec-node.cc@227
PS7, Line 227: null
> switch to nullptr while you're changing it in this file?
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/exec/exec-node.cc@228
PS7, Line 228: for (PlanNode* child : children_) {
> Maybe change to range-based for?
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h
File be/src/runtime/fragment-state.h:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h@19
PS7, Line 19:
> Use a #pragma once instead of the old-style included guards
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h@26
PS7, Line 26:
> This doesn't look like it's needed.
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h@32
PS7, Line 32:
> This doesn't look like it's needed.
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.h@56
PS7, Line 56: ig* sink_config() const { retur
> Might be better to use references, since they should be non-nullable. Not a
cpp doesn't allow the use of references in a vector
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.cc
File be/src/runtime/fragment-state.cc:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/fragment-state.cc@97
PS7, Line 97: codegen()->runtime_profile()->total_time_counter());
> Should use the same fragment ID format (F01, etc) as elsewhere in the profi
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.h@387
PS7, Line 387: /// StartFInstances();
> This
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@267
PS7, Line 267: int fragment_idx = -1;
> Does this need to be done in this patchset?
the TODO was an outdated comment, removed it.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@608
PS7, Line 608: DescriptorTbl::Create(&obj_pool_, query_ctx().desc_tbl_serialized, &desc_tbl_);
> nit: probably don't need this vertical whitespace.
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@612
PS7, Line 612:
> It would be be better to use the usual fragment ID (F01, etc), since that's
Done
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@621
PS7, Line 621: FragmentInstanceState* fis =
> This has the potential to change the timing of some queries significantly,
Like we discussed offline, I tried implementing a solution where in the fragment instance threads waited for codegen to complete. It worked fine but it changed the timing of the execution pipeline since the fragment instance threads were waiting for InitAndCodegen before it calls Prepare(). Since the query state waits for Prepare before sending runtime filters, what ended up happening was that even if the builder finished building runtime filters, it had to wait for the slowest fragment on the same host to complete codegen before query state marked Prepare as complete and sent out the filters. Because of this the filters were being sent out very slowly.
So, instead I switched the implementation to closely mimic the timing of current behavior and now codegen is done in FIS:open() and the first fragment instance to call codegen does the actual work while other instances wait. This change in timing also fixed one of the tests which was failing earlier (namely test_exchange_mem_usage_scaling) confirming our suspicion that the failure was due to a timing issue.
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/runtime/query-state.cc@622
PS7, Line 622: obj_pool_.Add(new FragmentInstanceState(this, fragment_state, *instance_ctx));
> I wonder about whether using a thread pool would be a good idea here, just
with my latest change, there is no need for spawning more threads
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf-internal.h
File be/src/udf/udf-internal.h:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf-internal.h@174
PS7, Line 174: static int GetConstFnAttr(bool uses_decimal_v2,
> Maybe pass in the query options struct?
I decided to just pass this bool here (just renamed it to uses_decimal_v2) to avoid adding adhoc class if IMPALA_UDF_SDK_BUILD is true
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf.cc
File be/src/udf/udf.cc:
http://gerrit.cloudera.org:8080/#/c/15408/7/be/src/udf/udf.cc@122
PS7, Line 122: #include "common/names.h"
> Does this need addressing?
This would be required if I pass a impala specific class to any of these methods here, since I decided to go with just passing the uses_decimal_v2 of type bool in FunctionContextImpl::GetConstFnAttr(), I wont need to define a new class when IMPALA_UDF_SDK_BUILD is true;
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 8
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Mar 2020 01:31:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080: Codegen once per fragment
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15408
to look at the new patch set (#9).
Change subject: IMPALA-4080: Codegen once per fragment
......................................................................
IMPALA-4080: Codegen once per fragment
This is the final patch:
- Removes all temporary changes that called codegen on plan nodes and
data sink through the exec node.
- Introduces FragmentState class that basically contains the plan tree,
data sink config and the codegen state for every fragment.
- Every FragmentState object has its own runtime profile that helps to
map out every codegen object's profile to its fragment id.
- Plan nodes are generated before fragment instance threads are spawned.
- The first fragment instance thread to call codegen on its parent
parent fragment does the actual codegen work and the rest of the
instance threads wait till codegen is complete.
- This very closely mimics the timing of the state machines both for
query state and the fragment instance states.
Testing:
Ran exhaustive tests successfully except for 2 tests.
TODO:
- Fix the failed tests namely:
test_under_statement_expression_limit - need to confirm if the
timing change fixed this too.
test_exchange_mem_usage_scaling - fixing the timing of the query
pipeline fixed this.
- Fix all TODOs, which are mostly adding comments
Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/blocking-join-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-avro-scanner.h
M be/src/exec/hdfs-columnar-scanner.cc
M be/src/exec/hdfs-columnar-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/join-builder.cc
M be/src/exec/join-builder.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.cc
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/exec/select-node.cc
M be/src/exec/select-node.h
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/subplan-node.cc
M be/src/exec/subplan-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/exprs/agg-fn.cc
M be/src/exprs/agg-fn.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
A be/src/runtime/fragment-state.cc
A be/src/runtime/fragment-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf.cc
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
100 files changed, 915 insertions(+), 625 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15408/9
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 9
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Mar 2020 21:01:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 13:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5597/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 13
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Apr 2020 18:23:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
IMPALA-4080 [part 7]: Codegen once per fragment
This is the final patch:
- Removes all temporary changes that called codegen on plan nodes and
data sink through the exec node.
- Introduces FragmentState class that basically contains the plan tree,
data sink config and the codegen state for every fragment.
- Every FragmentState object has its own runtime profile that helps to
map out every codegen object's profile to its fragment id.
- Plan nodes are generated before fragment instance threads are spawned.
- The first fragment instance thread to call codegen on its parent
parent fragment does the actual codegen work and the rest of the
instance threads wait till codegen is complete.
- This very closely mimics the timing of the state machines both for
query state and the fragment instance states.
Testing:
- Ran exhaustive tests successfully.
An example of how the codegen in the profile looks:
Per Node Profiles:
localhost:22002:
- AdmissionSlots: 1 (1)
- BloomFilterBytes: 16.00 MB (16777216)
- ScratchBytesRead: 0
- ScratchBytesWritten: 0
- ScratchFileUsedBytes: 0
- ScratchReads: 0 (0)
- ScratchWrites: 0 (0)
- TotalEncryptionTime: 0.000ns
- TotalReadBlockTime: 0.000ns
Buffer pool:
- AllocTime: 153.663us
- CumulativeAllocationBytes: 16.00 MB (16777216)
- CumulativeAllocations: 8 (8)
- PeakReservation: 16.00 MB (16777216)
- PeakUnpinnedBytes: 0
- PeakUsedReservation: 16.00 MB (16777216)
- ReadIoBytes: 0
- ReadIoOps: 0 (0)
- ReadIoWaitTime: 0.000ns
- ReservationLimit: 16.00 MB (16777216)
- SystemAllocTime: 133.841us
- WriteIoBytes: 0
- WriteIoOps: 0 (0)
- WriteIoWaitTime: 0.000ns
Fragment F02:
CodeGen:(Total: 695.207ms, non-child: 0.000ns, % non-child: 0.00%)
- CodegenInvoluntaryContextSwitches: 102 (102)
- CodegenTotalWallClockTime: 695.180ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 688.998ms
- CodegenVoluntaryContextSwitches: 22 (22)
- CompileTime: 39.657ms
- IrGenerationTime: 27.443ms
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.53 MB (2647876)
- NumFunctions: 161 (161)
- NumInstructions: 7.13K (7128)
- OptimizationTime: 596.075ms
- PeakMemoryUsage: 3.48 MB (3649536)
- PrepareTime: 30.782ms
Fragment F00:
CodeGen:(Total: 145.314ms, non-child: 0.000ns, % non-child: 0.00%)
- CodegenInvoluntaryContextSwitches: 11 (11)
- CodegenTotalWallClockTime: 145.304ms
- CodegenSysTime: 3.944ms
- CodegenUserTime: 137.738ms
- CodegenVoluntaryContextSwitches: 4 (4)
- CompileTime: 6.697ms
- IrGenerationTime: 3.173ms
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.53 MB (2647876)
- NumFunctions: 41 (41)
- NumInstructions: 1.02K (1024)
- OptimizationTime: 119.623ms
- PeakMemoryUsage: 512.00 KB (524288)
- PrepareTime: 14.405ms
Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Reviewed-on: http://gerrit.cloudera.org:8080/15408
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/blocking-join-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-avro-scanner.h
M be/src/exec/hdfs-columnar-scanner.cc
M be/src/exec/hdfs-columnar-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/join-builder.cc
M be/src/exec/join-builder.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.cc
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/exec/select-node.cc
M be/src/exec/select-node.h
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/subplan-node.cc
M be/src/exec/subplan-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/exprs/agg-fn.cc
M be/src/exprs/agg-fn.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
A be/src/runtime/fragment-state.cc
A be/src/runtime/fragment-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf.cc
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
100 files changed, 930 insertions(+), 625 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 14
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 13: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 13
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Apr 2020 22:51:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15408 )
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
Patch Set 11:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5592/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 11
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Mar 2020 21:05:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15408
to look at the new patch set (#10).
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
IMPALA-4080 [part 7]: Codegen once per fragment
This is the final patch:
- Removes all temporary changes that called codegen on plan nodes and
data sink through the exec node.
- Introduces FragmentState class that basically contains the plan tree,
data sink config and the codegen state for every fragment.
- Every FragmentState object has its own runtime profile that helps to
map out every codegen object's profile to its fragment id.
- Plan nodes are generated before fragment instance threads are spawned.
- The first fragment instance thread to call codegen on its parent
parent fragment does the actual codegen work and the rest of the
instance threads wait till codegen is complete.
- This very closely mimics the timing of the state machines both for
query state and the fragment instance states.
Testing:
- Ran exhaustive tests successfully.
An example of how the codegen in the profile looks:
Per Node Profiles:
localhost:22002:
- AdmissionSlots: 1 (1)
- BloomFilterBytes: 16.00 MB (16777216)
- ScratchBytesRead: 0
- ScratchBytesWritten: 0
- ScratchFileUsedBytes: 0
- ScratchReads: 0 (0)
- ScratchWrites: 0 (0)
- TotalEncryptionTime: 0.000ns
- TotalReadBlockTime: 0.000ns
Buffer pool:
- AllocTime: 153.663us
- CumulativeAllocationBytes: 16.00 MB (16777216)
- CumulativeAllocations: 8 (8)
- PeakReservation: 16.00 MB (16777216)
- PeakUnpinnedBytes: 0
- PeakUsedReservation: 16.00 MB (16777216)
- ReadIoBytes: 0
- ReadIoOps: 0 (0)
- ReadIoWaitTime: 0.000ns
- ReservationLimit: 16.00 MB (16777216)
- SystemAllocTime: 133.841us
- WriteIoBytes: 0
- WriteIoOps: 0 (0)
- WriteIoWaitTime: 0.000ns
Fragment F02:
CodeGen:(Total: 695.207ms, non-child: 0.000ns, % non-child: 0.00%)
- CodegenInvoluntaryContextSwitches: 102 (102)
- CodegenTotalWallClockTime: 695.180ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 688.998ms
- CodegenVoluntaryContextSwitches: 22 (22)
- CompileTime: 39.657ms
- IrGenerationTime: 27.443ms
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.53 MB (2647876)
- NumFunctions: 161 (161)
- NumInstructions: 7.13K (7128)
- OptimizationTime: 596.075ms
- PeakMemoryUsage: 3.48 MB (3649536)
- PrepareTime: 30.782ms
Fragment F00:
CodeGen:(Total: 145.314ms, non-child: 0.000ns, % non-child: 0.00%)
- CodegenInvoluntaryContextSwitches: 11 (11)
- CodegenTotalWallClockTime: 145.304ms
- CodegenSysTime: 3.944ms
- CodegenUserTime: 137.738ms
- CodegenVoluntaryContextSwitches: 4 (4)
- CompileTime: 6.697ms
- IrGenerationTime: 3.173ms
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.53 MB (2647876)
- NumFunctions: 41 (41)
- NumInstructions: 1.02K (1024)
- OptimizationTime: 119.623ms
- PeakMemoryUsage: 512.00 KB (524288)
- PrepareTime: 14.405ms
Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/blocking-join-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-avro-scanner.h
M be/src/exec/hdfs-columnar-scanner.cc
M be/src/exec/hdfs-columnar-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/join-builder.cc
M be/src/exec/join-builder.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.cc
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/exec/select-node.cc
M be/src/exec/select-node.h
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/subplan-node.cc
M be/src/exec/subplan-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/exprs/agg-fn.cc
M be/src/exprs/agg-fn.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
A be/src/runtime/fragment-state.cc
A be/src/runtime/fragment-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf.cc
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
100 files changed, 930 insertions(+), 625 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15408/10
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 7]: Codegen once per fragment
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15408
to look at the new patch set (#12).
Change subject: IMPALA-4080 [part 7]: Codegen once per fragment
......................................................................
IMPALA-4080 [part 7]: Codegen once per fragment
This is the final patch:
- Removes all temporary changes that called codegen on plan nodes and
data sink through the exec node.
- Introduces FragmentState class that basically contains the plan tree,
data sink config and the codegen state for every fragment.
- Every FragmentState object has its own runtime profile that helps to
map out every codegen object's profile to its fragment id.
- Plan nodes are generated before fragment instance threads are spawned.
- The first fragment instance thread to call codegen on its parent
parent fragment does the actual codegen work and the rest of the
instance threads wait till codegen is complete.
- This very closely mimics the timing of the state machines both for
query state and the fragment instance states.
Testing:
- Ran exhaustive tests successfully.
An example of how the codegen in the profile looks:
Per Node Profiles:
localhost:22002:
- AdmissionSlots: 1 (1)
- BloomFilterBytes: 16.00 MB (16777216)
- ScratchBytesRead: 0
- ScratchBytesWritten: 0
- ScratchFileUsedBytes: 0
- ScratchReads: 0 (0)
- ScratchWrites: 0 (0)
- TotalEncryptionTime: 0.000ns
- TotalReadBlockTime: 0.000ns
Buffer pool:
- AllocTime: 153.663us
- CumulativeAllocationBytes: 16.00 MB (16777216)
- CumulativeAllocations: 8 (8)
- PeakReservation: 16.00 MB (16777216)
- PeakUnpinnedBytes: 0
- PeakUsedReservation: 16.00 MB (16777216)
- ReadIoBytes: 0
- ReadIoOps: 0 (0)
- ReadIoWaitTime: 0.000ns
- ReservationLimit: 16.00 MB (16777216)
- SystemAllocTime: 133.841us
- WriteIoBytes: 0
- WriteIoOps: 0 (0)
- WriteIoWaitTime: 0.000ns
Fragment F02:
CodeGen:(Total: 695.207ms, non-child: 0.000ns, % non-child: 0.00%)
- CodegenInvoluntaryContextSwitches: 102 (102)
- CodegenTotalWallClockTime: 695.180ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 688.998ms
- CodegenVoluntaryContextSwitches: 22 (22)
- CompileTime: 39.657ms
- IrGenerationTime: 27.443ms
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.53 MB (2647876)
- NumFunctions: 161 (161)
- NumInstructions: 7.13K (7128)
- OptimizationTime: 596.075ms
- PeakMemoryUsage: 3.48 MB (3649536)
- PrepareTime: 30.782ms
Fragment F00:
CodeGen:(Total: 145.314ms, non-child: 0.000ns, % non-child: 0.00%)
- CodegenInvoluntaryContextSwitches: 11 (11)
- CodegenTotalWallClockTime: 145.304ms
- CodegenSysTime: 3.944ms
- CodegenUserTime: 137.738ms
- CodegenVoluntaryContextSwitches: 4 (4)
- CompileTime: 6.697ms
- IrGenerationTime: 3.173ms
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.53 MB (2647876)
- NumFunctions: 41 (41)
- NumInstructions: 1.02K (1024)
- OptimizationTime: 119.623ms
- PeakMemoryUsage: 512.00 KB (524288)
- PrepareTime: 14.405ms
Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/blocking-join-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-avro-scanner.h
M be/src/exec/hdfs-columnar-scanner.cc
M be/src/exec/hdfs-columnar-scanner.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-sequence-scanner.cc
M be/src/exec/hdfs-sequence-scanner.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/hdfs-text-scanner.h
M be/src/exec/join-builder.cc
M be/src/exec/join-builder.h
M be/src/exec/nested-loop-join-builder.cc
M be/src/exec/nested-loop-join-builder.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.cc
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/exec/select-node.cc
M be/src/exec/select-node.h
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/subplan-node.cc
M be/src/exec/subplan-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/exprs/agg-fn.cc
M be/src/exprs/agg-fn.h
M be/src/exprs/expr-codegen-test.cc
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
A be/src/runtime/fragment-state.cc
A be/src/runtime/fragment-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/udf/udf.cc
M be/src/util/tuple-row-compare.cc
M be/src/util/tuple-row-compare.h
100 files changed, 930 insertions(+), 625 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15408/12
--
To view, visit http://gerrit.cloudera.org:8080/15408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3aef8bc621f96caafe9a1c378617a2987e4ad452
Gerrit-Change-Number: 15408
Gerrit-PatchSet: 12
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>