Tim Armstrong has posted comments on this change. (
Change subject: IMPALA-6120: Add thread timers for reporting codegen time
Patch Set 1:
There might be some opportunities to further simplify these counters and make
them easier to understand.
PS1, Line 209: LlvmThread
Maybe just "Codegen" works as a prefix? "LlvmThread" sort-of implies that it's
a separate thread.
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.
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.
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-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 Apr 2018 00:16:09 +0000