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 <[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 16:44:04 +0000 Gerrit-HasComments: Yes
