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 <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
Gerrit-HasComments: Yes

Reply via email to