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 <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 10 Apr 2018 00:16:09 +0000 Gerrit-HasComments: Yes
