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 <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 11 Apr 2018 01:28:47 +0000 Gerrit-HasComments: Yes
