Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/8758 )
Change subject: IMPALA-6190/6246: Add instances tab and event sequence ...................................................................... Patch Set 7: (18 comments) Thank you for the review. Please see PS7. http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/coordinator.h@187 PS4, Line 187: instances > instances' stats Done http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.cc@241 PS3, Line 241: // Release the thread token on the ro > Please see comments in Open(). This state doesn't seem necessary anymore if Done http://gerrit.cloudera.org:8080/#/c/8758/3/be/src/runtime/fragment-instance-state.cc@302 PS3, Line 302: SCOPED_TIMER(ADD_CHILD_TIMER(timings_profile_, "ExecTreeOpenTime", OPEN_TIMER_NAME)); : RETURN_IF_ERROR(exec_tree_->Open(runtime_state_)); : } : return sink_->Open(runtime_state_); : } : : Status FragmentInstanceState::ExecInternal() { : RuntimeProfile::Counter* plan_exec_timer = : ADD_CHILD_TIMER(timings_profile_, "ExecTreeExecTime", EXEC_TIMER_NAME); : SCOPED_THREAD_COUNTER_MEASUREMENT(runt > I think it's definitely worth doing a perf run to make sure things don't re I will kick of a perf run and will include the results in the commit message. Regarding your second point, I'm not sure I understand your idea. Do you think the compiler should inline the relevant parts from the switch? http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@63 PS4, Line 63: /// Events that are tracked in a separate timeline for each fragment instance. : static const string INSTANCE_EVENT_TIMELINE_NAME = : "Fragment Instance Lifecycle Event Timeline"; : : /// Indicates that the call to Prepare() has been completed. : static const string PREPARE_EVENT_NAME = "Prepare Finished"; : : /// Indicates that the call to Open() has been completed. > Please provide comments about these names and how they may correlate with t Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@73 PS4, Line 73: he first row batch has b > Is there any static assert to ensure that number of entries in this array m I added one that will break if we only change one and then will require updating, too. I'm not particularly happy with it since it's somewhat ungeneric, but I couldn't find a way to get the size of the Thrift enum during compile time. However, it'll break if we only change one so it does what it should. http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@79 PS4, Line 79: > Will "produced" be more accurate as there is usually some extra processing Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@81 PS4, Line 81: bout to be sen > "Producing row batches" seems more consistent with the rest of the state na Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@283 PS4, Line 283: > This is actually the majority of time codegen spent on. May want to extend Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@302 PS4, Line 302: OPED_TIMER(ADD_CHILD_TIMER(timings_profile_ > Can this be moved out of the loop to line 297 and simplify the state machin Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@306 PS4, Line 306: > Should we not count the time to execute UpdateState() towards "plan_exec_ti Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@410 PS4, Line 410: VLOG_FILE << "Reporting " << (done ? "final " : "") << "profile for instance " > const StateEvent Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@418 PS4, Line 418: (per_h > DCHECK_EQ(). Same below. Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@483 PS4, Line 483: // Allo > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@484 PS4, Line 484: event_sequence_->MarkEvent(EXEC_INTERNAL_EVENT_NAME); > Does it help to print "event" too ? Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/runtime/fragment-instance-state.cc@487 PS4, Line 487: > Can there be multiple threads updating the state here ? If so, does the abo There are no updating threads, but there's still a race between updating it here and reading it in the reporting thread. As discussed in person, I'll keep the AtomicEnum to make the behavior explicitly correct. The comment of 'current_state_' in the header explains where it is written and read. http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/util/runtime-profile-counters.h@305 PS4, Line 305: Event event = make_pair(move(label), sw_.ElapsedTime()); : boost::lock_guard<SpinLock> event_lock(lock_); > Why the change in order ? Do you mean to count the spin lock wait time towa My thought was that that would keep the list sorted. You're right though, we don't want to include the lock time so I reverted this. http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/util/runtime-profile-counters.h@333 PS4, Line 333: DCHECK_ > DCHECK_EQ Done http://gerrit.cloudera.org:8080/#/c/8758/4/be/src/util/runtime-profile-counters.h@337 PS4, Line 337: > Not sure if the compiler is smart enough to hoist this out of the loop as i Done -- To view, visit http://gerrit.cloudera.org:8080/8758 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I626456b6afa9101eeeeffd5cda10c4096d63d7f9 Gerrit-Change-Number: 8758 Gerrit-PatchSet: 7 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Comment-Date: Wed, 13 Dec 2017 19:40:51 +0000 Gerrit-HasComments: Yes
