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 2018/04/09 23:47:32 UTC
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9960
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
IMPALA-6120: Add thread timers for reporting codegen time
Add thread times for accurate reporting of codegen time.
Also cleaned up a few places where time elapsed was being counted twice.
Sample Profile:
Query: SELECT count(*) FROM tpch_parquet.lineitem
WHERE l_partkey in (1,6,11,16,21,26,31,36,41);
CodeGen:(Total: 35.377ms, non-child: 35.377ms, % non-child: 100.00%)
- CodegenTime: 328.869us
- CompileTime: 2.163ms
- LlvmThreadInvoluntaryContextSwitches: 6 (6)
- LlvmThreadTotalWallClockTime: 35.698ms
- LlvmThreadSysTime: 3.584ms
- LlvmThreadUserTime: 29.555ms
- LlvmThreadVoluntaryContextSwitches: 0 (0)
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 21.881ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 11.147ms
Sample Profile with an added 2 sec sleep time to "OptimizationTime":
CodeGen:(Total: 2s037ms, non-child: 2s037ms, % non-child: 100.00%)
- CodegenTime: 386.948us
- CompileTime: 2.032ms
- LlvmThreadInvoluntaryContextSwitches: 0 (0)
- LlvmThreadTotalWallClockTime: 2s037ms
- LlvmThreadSysTime: 0.000ns
- LlvmThreadUserTime: 37.672ms
- LlvmThreadVoluntaryContextSwitches: 1 (1)
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 2s023ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 11.598ms
Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/runtime/tuple.cc
M be/src/util/tuple-row-compare.cc
10 files changed, 26 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/9960/1
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
Carrying over Tim's +2
http://gerrit.cloudera.org:8080/#/c/9960/4/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/9960/4/be/src/codegen/llvm-codegen.h@758
PS4, Line 758: N_STA
> Is OPEN a phase or a state? Not sure what terminology we use, or if we're c
Turns out we do define states and this corresponds to "CODEGEN_START" state. See https://github.com/apache/impala/blob/818cd8fa2721cd8205b304563b728952bffc8b2f/be/src/runtime/fragment-instance-state.h#L186
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 01:11:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9960/4/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/9960/4/be/src/codegen/llvm-codegen.h@758
PS4, Line 758: state
Is OPEN a phase or a state? Not sure what terminology we use, or if we're consistent.
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Apr 2018 22:27:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 6: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2300/
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 23:54:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2300/
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 20:08:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 1:
(4 comments)
There might be some opportunities to further simplify these counters and make them easier to understand.
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@209
PS1, Line 209: LlvmThread
Maybe just "Codegen" works as a prefix? "LlvmThread" sort-of implies that it's a separate thread.
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@329
PS1, Line 329: SCOPED_TIMER(prepare_module_timer_);
Maybe this should just include this whole function? Since it is essentially preparing the module.
.. Although, see my later comments - maybe counting this as preparation doesn't make sense at all.
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@657
PS1, Line 657: SCOPED_TIMER(profile_->total_time_counter());
I'm not sure that the timers here make sense - I think we may be double-counting, since this is now only called from GetFunction(), which I believe should only be called during the "codegen" phase, when we already want to have total_time_counter() and llvm_thread_counters_ running anyway. I.e. maybe we only need to have prepare_module_timer_ running here.
More radically, I'm not sure how useful the Prepare/Codegen time distinction is, since it's confusing with lazy materialization.
Maybe it would be cleaner to have Prepare only include CreateImpalaCodegen, i.e. the initial module creation, and Codegen to include the whole Codegen phase, with no overlap.
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/exec/hdfs-parquet-scanner.cc@1101
PS1, Line 1101: SCOPED_TIMER(codegen->codegen_timer());
We could switch to starting these timers in FragmentInstanceState::Open(), since all of this codegen for the plan tree now happens in one go.
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Apr 2018 00:16:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 7: Code-Review+2
GVO failed due to flakiness addressed in IMPALA-6092 (thanks to Vuk for pointing that out). Rebasing to get the fix for that and re-running the GVO.
Carrying over +2
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 22:25:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9960
to look at the new patch set (#4).
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
IMPALA-6120: Add thread timers for reporting codegen time
Add thread times for accurate reporting of codegen time.
Also cleaned up a few places where time elapsed was being counted twice.
Sample Profile:
Query: SELECT count(*) FROM tpch_parquet.lineitem
WHERE l_partkey in (1,6,11,16,21,26,31,36,41);
CodeGen:(Total: 37.948ms, non-child: 37.948ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 37.942ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 36.938ms
- CodegenVoluntaryContextSwitches: 0 (0)
- CompileTime: 2.065ms
- IrGenerationTime: 392.351us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 21.416ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 13.496ms
Sample Profile with an added 2 sec sleep time to "OptimizationTime":
CodeGen:(Total: 2s037ms, non-child: 2s037ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 2s037ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 37.672ms
- CodegenVoluntaryContextSwitches: 1 (1)
- CompileTime: 2.032ms
- IrGenerationTime: 386.948us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 2s023ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 11.598ms
Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/tuple.cc
M be/src/util/tuple-row-compare.cc
11 files changed, 26 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/9960/4
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9960
to look at the new patch set (#5).
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
IMPALA-6120: Add thread timers for reporting codegen time
Add thread times for accurate reporting of codegen time.
Also cleaned up a few places where time elapsed was being counted twice.
Sample Profile:
Query: SELECT count(*) FROM tpch_parquet.lineitem
WHERE l_partkey in (1,6,11,16,21,26,31,36,41);
CodeGen:(Total: 37.948ms, non-child: 37.948ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 37.942ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 36.938ms
- CodegenVoluntaryContextSwitches: 0 (0)
- CompileTime: 2.065ms
- IrGenerationTime: 392.351us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 21.416ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 13.496ms
Sample Profile with an added 2 sec sleep time to "OptimizationTime":
CodeGen:(Total: 2s037ms, non-child: 2s037ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 2s037ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 37.672ms
- CodegenVoluntaryContextSwitches: 1 (1)
- CompileTime: 2.032ms
- IrGenerationTime: 386.948us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 2s023ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 11.598ms
Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/tuple.cc
M be/src/util/tuple-row-compare.cc
11 files changed, 26 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/9960/5
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9960
to look at the new patch set (#2).
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
IMPALA-6120: Add thread timers for reporting codegen time
Add thread times for accurate reporting of codegen time.
Also cleaned up a few places where time elapsed was being counted twice.
Sample Profile:
Query: SELECT count(*) FROM tpch_parquet.lineitem
WHERE l_partkey in (1,6,11,16,21,26,31,36,41);
CodeGen:(Total: 34.660ms, non-child: 34.660ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 4 (4)
- CodegenTime: 339.399us
- CodegenTotalWallClockTime: 34.653ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 30.656ms
- CodegenVoluntaryContextSwitches: 0 (0)
- CompileTime: 2.365ms
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 21.167ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 10.020ms
Sample Profile with an added 2 sec sleep time to "OptimizationTime":
CodeGen:(Total: 2s037ms, non-child: 2s037ms, % non-child: 100.00%)
- CodegenTime: 386.948us
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 2s037ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 37.672ms
- CodegenVoluntaryContextSwitches: 1 (1)
- CompileTime: 2.032ms
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 2s023ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 11.598ms
Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/tuple.cc
M be/src/util/tuple-row-compare.cc
11 files changed, 21 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/9960/2
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
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/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
IMPALA-6120: Add thread timers for reporting codegen time
Add thread times for accurate reporting of codegen time.
Also cleaned up a few places where time elapsed was being counted twice.
Sample Profile:
Query: SELECT count(*) FROM tpch_parquet.lineitem
WHERE l_partkey in (1,6,11,16,21,26,31,36,41);
CodeGen:(Total: 37.948ms, non-child: 37.948ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 37.942ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 36.938ms
- CodegenVoluntaryContextSwitches: 0 (0)
- CompileTime: 2.065ms
- IrGenerationTime: 392.351us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 21.416ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 13.496ms
Sample Profile with an added 2 sec sleep time to "OptimizationTime":
CodeGen:(Total: 2s037ms, non-child: 2s037ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 2s037ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 37.672ms
- CodegenVoluntaryContextSwitches: 1 (1)
- CompileTime: 2.032ms
- IrGenerationTime: 386.948us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 2s023ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 11.598ms
Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Reviewed-on: http://gerrit.cloudera.org:8080/9960
Reviewed-by: Bikramjeet Vig <bi...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/tuple.cc
M be/src/util/tuple-row-compare.cc
11 files changed, 30 insertions(+), 39 deletions(-)
Approvals:
Bikramjeet Vig: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
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: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 6:
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/2308/
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 21:53:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9960
to look at the new patch set (#3).
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
IMPALA-6120: Add thread timers for reporting codegen time
Add thread times for accurate reporting of codegen time.
Also cleaned up a few places where time elapsed was being counted twice.
Sample Profile:
Query: SELECT count(*) FROM tpch_parquet.lineitem
WHERE l_partkey in (1,6,11,16,21,26,31,36,41);
CodeGen:(Total: 37.948ms, non-child: 37.948ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 37.942ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 36.938ms
- CodegenVoluntaryContextSwitches: 0 (0)
- CompileTime: 2.065ms
- IrGenerationTime: 392.351us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 21.416ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 13.496ms
Sample Profile with an added 2 sec sleep time to "OptimizationTime":
CodeGen:(Total: 2s037ms, non-child: 2s037ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 2s037ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 37.672ms
- CodegenVoluntaryContextSwitches: 1 (1)
- CompileTime: 2.032ms
- IrGenerationTime: 386.948us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 2s023ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 11.598ms
Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/tuple.cc
M be/src/util/tuple-row-compare.cc
11 files changed, 27 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/9960/3
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.h@761
PS1, Line 761: RuntimeProfile::Counter* optimization_timer_;
> If we switch to having the counter updated outside of this class, lets docu
Done
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.h@779
PS1, Line 779:
> If we switch to having the counter updated outside of this class, lets docu
Done
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@204
PS1, Line 204: CodegenTime
> Yeah IrGenerationTime or something like that would be more accurate
Done
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@216
PS1, Line 216: SCOPED_TIMER((*codegen)->profile_->total_time_counter());
: SCOPED_THREAD_COUNTER_MEASUREMENT((*codegen)->llvm_thread_counters());
> I think we should keep this consistent with CreateFromMemory() to minimise
got it. I ll leave this in and remove the timer from LinkModuleFromLocalFs()
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Apr 2018 22:12:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9960
to look at the new patch set (#6).
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
IMPALA-6120: Add thread timers for reporting codegen time
Add thread times for accurate reporting of codegen time.
Also cleaned up a few places where time elapsed was being counted twice.
Sample Profile:
Query: SELECT count(*) FROM tpch_parquet.lineitem
WHERE l_partkey in (1,6,11,16,21,26,31,36,41);
CodeGen:(Total: 37.948ms, non-child: 37.948ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 37.942ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 36.938ms
- CodegenVoluntaryContextSwitches: 0 (0)
- CompileTime: 2.065ms
- IrGenerationTime: 392.351us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 21.416ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 13.496ms
Sample Profile with an added 2 sec sleep time to "OptimizationTime":
CodeGen:(Total: 2s037ms, non-child: 2s037ms, % non-child: 100.00%)
- CodegenInvoluntaryContextSwitches: 0 (0)
- CodegenTotalWallClockTime: 2s037ms
- CodegenSysTime: 0.000ns
- CodegenUserTime: 37.672ms
- CodegenVoluntaryContextSwitches: 1 (1)
- CompileTime: 2.032ms
- IrGenerationTime: 386.948us
- LoadTime: 0.000ns
- ModuleBitcodeSize: 2.26 MB (2373148)
- NumFunctions: 22 (22)
- NumInstructions: 381 (381)
- OptimizationTime: 2s023ms
- PeakMemoryUsage: 190.50 KB (195072)
- PrepareTime: 11.598ms
Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
---
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/select-node.cc
M be/src/exec/text-converter.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/tuple.cc
M be/src/util/tuple-row-compare.cc
11 files changed, 30 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/9960/6
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 6: Code-Review+2
Carrying over +2
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 20:08:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2312/
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 22:26:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 6:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2308/
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 18:01:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@201
PS1, Line 201: load_module_timer_ = ADD_TIMER(profile_, "LoadTime");
this is only used in a method which is used as a part of a temp codegen object which does not care about timers. Unless we have any reason to believe that this can be a problem in the future and would help diagnose it, we should get rid of this. or else output it somewhere
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@204
PS1, Line 204: CodegenTime
this counts the time taken to adding IRs to the module, can be named better as CodegenTime implies the whole process of codegen. Off the top of my head, how about something like "ir_generation_timer". open to better suggestions!
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@209
PS1, Line 209: LlvmThread
> Maybe just "Codegen" works as a prefix? "LlvmThread" sort-of implies that i
Done
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@216
PS1, Line 216: SCOPED_TIMER((*codegen)->profile_->total_time_counter());
: SCOPED_THREAD_COUNTER_MEASUREMENT((*codegen)->llvm_thread_counters());
Maybe remove this? CreateFromFile is used in a temp codegen object which does not care about timers.
Same for LinkModuleFromLocalFs
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@311
PS1, Line 311: profile_->total_time_counter()
adding this timer to wherever codegen_timer_ is used?
This way we have parity between the total wall clock time counted by llvm_thread_counters_ and profile_->total_time_counter
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@329
PS1, Line 329: SCOPED_TIMER(prepare_module_timer_);
> Maybe this should just include this whole function? Since it is essentially
removed according to your later comment.
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@657
PS1, Line 657: SCOPED_TIMER(profile_->total_time_counter());
> I'm not sure that the timers here make sense - I think we may be double-cou
Done. Only included Prepare in Codegen
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/exec/hdfs-parquet-scanner.cc@1101
PS1, Line 1101: SCOPED_TIMER(codegen->codegen_timer());
> We could switch to starting these timers in FragmentInstanceState::Open(),
Absolutely Agree, I was initially thinking of doing this at the top level Codegen method in every exec node, but this sounds even better
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Apr 2018 01:28:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 7
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 14 Apr 2018 02:21:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9960/6/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:
http://gerrit.cloudera.org:8080/#/c/9960/6/be/src/runtime/fragment-instance-state.cc@256
PS6, Line 256: RETURN_IF_ERROR(runtime_state_->CodegenScalarFns());
missed to include this
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 12 Apr 2018 01:18:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.h@761
PS1, Line 761: RuntimeProfile::Counter* optimization_timer_;
If we switch to having the counter updated outside of this class, lets document here how we expect it to be updated.
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.h@779
PS1, Line 779:
If we switch to having the counter updated outside of this class, lets document here how we expect it to be updated.
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@201
PS1, Line 201: load_module_timer_ = ADD_TIMER(profile_, "LoadTime");
> this is only used in a method which is used as a part of a temp codegen obj
I think this should include time spent loading IR UDFs from files on disk. It's probably not useful most of the time, but could tell us if the filesystem is the problem (e.g. loading from a slow filesystem or disk).
The value is marginal though.
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@204
PS1, Line 204: CodegenTime
> this counts the time taken to adding IRs to the module, can be named better
Yeah IrGenerationTime or something like that would be more accurate
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@216
PS1, Line 216: SCOPED_TIMER((*codegen)->profile_->total_time_counter());
: SCOPED_THREAD_COUNTER_MEASUREMENT((*codegen)->llvm_thread_counters());
> Maybe remove this? CreateFromFile is used in a temp codegen object which do
I think we should keep this consistent with CreateFromMemory() to minimise potential confusion, since thye are analogous.
It looks like LinkModuleFromLocalFs() is only called during the IR generation phase, so I think the time is actually already being counted.
http://gerrit.cloudera.org:8080/#/c/9960/1/be/src/codegen/llvm-codegen.cc@311
PS1, Line 311: profile_->total_time_counter()
> adding this timer to wherever codegen_timer_ is used?
Yeah I think those two timers should be running for exactly the same time.
http://gerrit.cloudera.org:8080/#/c/9960/2/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/9960/2/be/src/codegen/llvm-codegen.cc@311
PS2, Line 311: SCOPED_TIMER(profile_->total_time_counter());
I think this function is only called during the IR generation phase, so these timers should already be running when this function is called.
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Apr 2018 16:44:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6120: Add thread timers for reporting codegen time
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/9960 )
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
......................................................................
Patch Set 6:
GVO failed due to an unrelated flaky test IMPALA-1995
--
To view, visit http://gerrit.cloudera.org:8080/9960
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I24d5a46b8870bc959b89045432d2e86af72b30e5
Gerrit-Change-Number: 9960
Gerrit-PatchSet: 6
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Apr 2018 18:00:18 +0000
Gerrit-HasComments: No