Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
273103003:
Add responsiveness_metric for timeline_based_measurement (Closed)
|
Created:
6 years, 7 months ago by nednguyen Modified:
6 years, 6 months ago CC:
chromium-reviews, tfarina, telemetry+watch_chromium.org, ryoji_google.com, ruilopes Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd responsiveness_metric to timeline_based_measurement.
responsiveness_metric is a timeline_based_metric.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275036
Patch Set 1 #
Total comments: 43
Patch Set 2 : Address Chris' comments #
Total comments: 2
Patch Set 3 : Rebase after partial lands #
Total comments: 8
Patch Set 4 : Address Chris's comments #
Total comments: 6
Patch Set 5 : Address Nat's comments #Patch Set 6 : Rebase after partial lands. #
Total comments: 2
Patch Set 7 : Add TODO for fixing warning in repsonsiveness_metric #Patch Set 8 : Disable timeline_based_measurement smoke test for responsive on windows platform #
Messages
Total messages: 30 (0 generated)
I have a lot of questions, mostly since I'm still getting around to understand the code. Please be patient with me! (: https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/timeline/event.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:6: """Represents a timeline event. Can we clarify what a timeline event means? This may clarify one confusion I have: why is there only 1 thread_start/thread_duration per event (can't it have multiple of them)? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:10: (ticking when the thread is actually scheduled). Thread time is optional (Btw, what does thread time referring to here? Just thread_duration or all 3 of the thread timing? Maybe clarify the doc.) https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:25: 2 blank lines intentional? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:40: def IsTopMessageLoop(self): Document what a top message loop means? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:49: overlapped_interval_thread_start = max(self.thread_start, thread_start) This doesn't specifically handle None? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:52: overlapped_interval_thread_end - overlapped_interval_thread_start, 0) nit: 4-space align? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/timeline/thread.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/thread.py:59: range (start, end). "Returns all slices with time ranges within start and end, inclusive." (Btw, do we follow google python style guide here or something else? If so, this should be just 1 line, with additional information in the next paragraph.) https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/thread.py:68: (start, end). thread_start, thread_end https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/thread.py:71: if s.thread_end > thread_start and s.thread_start < thread_end: Slice objects are TimelineEvent (right?), so you will need to handle None here. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:1: # Copyright 2014 The Chromium Authors. All rights reserved. nit: Filename should probably be main_thread_jank to be consistent. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:8: PERCEIVABLE_DELAY_MS = 50 Please doc this. Maybe name it as USER_PERCEIVABLE_DELAY_THRESHOLD_MS https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:13: self.sum_top_slices_thread_time = 0 Hm, slightly inconsistent. We seem to use top slices and top message loop for the same thing in this file. Should this be top_message_loop? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:17: self.biggest_top_slice_thread_time = 0 Document each of this stats (if not here, somewhere else where it makes sense to document). https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:21: for s in renderer_thread.IterAllSlicesOverlappedWithThreadTimeRange( Can you explain again as to the difference between start/end and thread_start/thread_end? I had thought that thread_start/thread_end is when the event is actually being scheduled by the kernel (and start, end is the actual wall time). If that's the case, shouldn't this be using the start/end instead of thread_start/thread_end? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:22: thread_start, thread_end): 4-col indent. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:23: if not s.IsTopMessageLoop(): Btw, maybe this should be a static function instead of a member function of TimelineEvent. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:25: jank_thread_duration = s.GetOverlappedThreadTimeDurationWithRange( Question on thread_time/thread_end applies here too. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:36: class MainthreadJankMetric(timeline_based_metric.TimelineBasedMetric): This is inconsistent, above you use MainThread, and here Mainthread (hence also my comment on file name). We should pick one and stick with it. (: https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:41: self.VerifyNonOverlappedRecords(interaction_records) Why must the records be non-overlapping? Shouldn't we be outputting results on per-timeline record basis instead of a combined results? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:49: renderer_thread, record.thread_start, record.thread_end) 4-col hanging indent, same everywhere below too. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:49: renderer_thread, record.thread_start, record.thread_end) renderer_thread isn't documented even in the base class, we should document what it means. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... File tools/telemetry/telemetry/web_perf/metrics/unittest_util.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/unittest_util.py:19: Are the 2 blank lines intentional? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:89: event.thread_start, event.thread_end) align after (
https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/timeline/event.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:6: """Represents a timeline event. On 2014/05/13 03:22:38, chrishenry wrote: > Can we clarify what a timeline event means? This may clarify one confusion I > have: why is there only 1 thread_start/thread_duration per event (can't it have > multiple of them)? I think the notion of trace event is documented here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Maybe referencing this link here will be enough? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:10: (ticking when the thread is actually scheduled). Thread time is optional On 2014/05/13 03:22:38, chrishenry wrote: > (Btw, what does thread time referring to here? Just thread_duration or all 3 of > the thread timing? Maybe clarify the doc.) Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:25: On 2014/05/13 03:22:38, chrishenry wrote: > 2 blank lines intentional? Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:49: overlapped_interval_thread_start = max(self.thread_start, thread_start) On 2014/05/13 03:22:38, chrishenry wrote: > This doesn't specifically handle None? Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:52: overlapped_interval_thread_end - overlapped_interval_thread_start, 0) On 2014/05/13 03:22:38, chrishenry wrote: > nit: 4-space align? Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/timeline/thread.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/thread.py:59: range (start, end). On 2014/05/13 03:22:38, chrishenry wrote: > "Returns all slices with time ranges within start and end, inclusive." > > (Btw, do we follow google python style guide here or something else? If so, this > should be just 1 line, with additional information in the next paragraph.) Done. This weird newline is because I used a splitted screen in my terminal. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/thread.py:71: if s.thread_end > thread_start and s.thread_start < thread_end: On 2014/05/13 03:22:38, chrishenry wrote: > Slice objects are TimelineEvent (right?), so you will need to handle None here. Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:8: PERCEIVABLE_DELAY_MS = 50 On 2014/05/13 03:22:38, chrishenry wrote: > Please doc this. Maybe name it as USER_PERCEIVABLE_DELAY_THRESHOLD_MS Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:13: self.sum_top_slices_thread_time = 0 On 2014/05/13 03:22:38, chrishenry wrote: > Hm, slightly inconsistent. We seem to use top slices and top message loop for > the same thing in this file. Should this be top_message_loop? Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:21: for s in renderer_thread.IterAllSlicesOverlappedWithThreadTimeRange( On 2014/05/13 03:22:38, chrishenry wrote: > Can you explain again as to the difference between start/end and > thread_start/thread_end? I had thought that thread_start/thread_end is when the > event is actually being scheduled by the kernel (and start, end is the actual > wall time). If that's the case, shouldn't this be using the start/end instead of > thread_start/thread_end? Since what happens outside of the browser is uncontrollable by us, so we want to denoise the metrics by using thread_start, thread_end. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:22: thread_start, thread_end): On 2014/05/13 03:22:38, chrishenry wrote: > 4-col indent. Done. Just realize that my vim indent for python was messed up. :-) https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:23: if not s.IsTopMessageLoop(): On 2014/05/13 03:22:38, chrishenry wrote: > Btw, maybe this should be a static function instead of a member function of > TimelineEvent. Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:25: jank_thread_duration = s.GetOverlappedThreadTimeDurationWithRange( On 2014/05/13 03:22:38, chrishenry wrote: > Question on thread_time/thread_end applies here too. Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:36: class MainthreadJankMetric(timeline_based_metric.TimelineBasedMetric): On 2014/05/13 03:22:38, chrishenry wrote: > This is inconsistent, above you use MainThread, and here Mainthread (hence also > my comment on file name). We should pick one and stick with it. (: Picked Mainthread. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:41: self.VerifyNonOverlappedRecords(interaction_records) On 2014/05/13 03:22:38, chrishenry wrote: > Why must the records be non-overlapping? Shouldn't we be outputting results on > per-timeline record basis instead of a combined results? This is supposed to return a summarized results for multiple records. To get one results for each record, one needs to call this multiple time (and timeline_based_measurement does so). https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:49: renderer_thread, record.thread_start, record.thread_end) On 2014/05/13 03:22:38, chrishenry wrote: > renderer_thread isn't documented even in the base class, we should document what > it means. Ok. Will escalate this to Nat, who knows about this better than me. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... File tools/telemetry/telemetry/web_perf/metrics/unittest_util.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/unittest_util.py:19: On 2014/05/13 03:22:38, chrishenry wrote: > Are the 2 blank lines intentional? Done. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:89: event.thread_start, event.thread_end) On 2014/05/13 03:22:38, chrishenry wrote: > align after ( Done.
Nat or Tony, can you have a look at this too? I call out one comment below on whether a single event can have multiple thread_start/thread_end. Assuming a single event can only have a single thread_start/thread_end, code looks good to me. https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/timeline/event.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/timeline/event.py:6: """Represents a timeline event. On 2014/05/14 17:32:27, nednguyen wrote: > On 2014/05/13 03:22:38, chrishenry wrote: > > Can we clarify what a timeline event means? This may clarify one confusion I > > have: why is there only 1 thread_start/thread_duration per event (can't it > have > > multiple of them)? > > I think the notion of trace event is documented here: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Maybe referencing this link here will be enough? Maybe a link and a summary? I read thru TraceEvent.h briefly and didn't find anything that actually answer my real question: can we have multiple thread_start/thread_end within a single event? In fact, reading the documentation raised another question: event can be asynchronous, how does it associate what work (cpu time) is associated with such asynchronous event (it probably doesn't do that?)? https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:21: for s in renderer_thread.IterAllSlicesOverlappedWithThreadTimeRange( On 2014/05/14 17:32:27, nednguyen wrote: > On 2014/05/13 03:22:38, chrishenry wrote: > > Can you explain again as to the difference between start/end and > > thread_start/thread_end? I had thought that thread_start/thread_end is when > the > > event is actually being scheduled by the kernel (and start, end is the actual > > wall time). If that's the case, shouldn't this be using the start/end instead > of > > thread_start/thread_end? > Since what happens outside of the browser is uncontrollable by us, so we want to > denoise the metrics by using thread_start, thread_end. Ok, I agree. https://codereview.chromium.org/273103003/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): https://codereview.chromium.org/273103003/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:12: USER_PERCEIVABLE_DELAYS_THRESHOLD_MS = 50 s/DELAYS/DELAY/ https://codereview.chromium.org/273103003/diff/20001/tools/telemetry/unittest... File tools/telemetry/unittest_data/interaction_enabled_page.html (right): https://codereview.chromium.org/273103003/diff/20001/tools/telemetry/unittest... tools/telemetry/unittest_data/interaction_enabled_page.html:57: <script> Should this be in <body>?
On 2014/05/19 23:16:50, chrishenry wrote: > Nat or Tony, can you have a look at this too? I call out one comment below on > whether a single event can have multiple thread_start/thread_end. > > Assuming a single event can only have a single thread_start/thread_end, code > looks good to me. > > https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/timeline/event.py (right): > > https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/timeline/event.py:6: """Represents a timeline > event. > On 2014/05/14 17:32:27, nednguyen wrote: > > On 2014/05/13 03:22:38, chrishenry wrote: > > > Can we clarify what a timeline event means? This may clarify one confusion I > > > have: why is there only 1 thread_start/thread_duration per event (can't it > > have > > > multiple of them)? > > > > I think the notion of trace event is documented here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Maybe referencing this link here will be enough? > > Maybe a link and a summary? I read thru TraceEvent.h briefly and didn't find > anything that actually answer my real question: can we have multiple > thread_start/thread_end within a single event? > > In fact, reading the documentation raised another question: event can be > asynchronous, how does it associate what work (cpu time) is associated with such > asynchronous event (it probably doesn't do that?)? > > https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... > File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): > > https://codereview.chromium.org/273103003/diff/1/tools/telemetry/telemetry/we... > tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:21: for s in > renderer_thread.IterAllSlicesOverlappedWithThreadTimeRange( > On 2014/05/14 17:32:27, nednguyen wrote: > > On 2014/05/13 03:22:38, chrishenry wrote: > > > Can you explain again as to the difference between start/end and > > > thread_start/thread_end? I had thought that thread_start/thread_end is when > > the > > > event is actually being scheduled by the kernel (and start, end is the > actual > > > wall time). If that's the case, shouldn't this be using the start/end > instead > > of > > > thread_start/thread_end? > > Since what happens outside of the browser is uncontrollable by us, so we want > to > > denoise the metrics by using thread_start, thread_end. > > Ok, I agree. > > https://codereview.chromium.org/273103003/diff/20001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): > > https://codereview.chromium.org/273103003/diff/20001/tools/telemetry/telemetr... > tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:12: > USER_PERCEIVABLE_DELAYS_THRESHOLD_MS = 50 > s/DELAYS/DELAY/ Done. > https://codereview.chromium.org/273103003/diff/20001/tools/telemetry/unittest... > File tools/telemetry/unittest_data/interaction_enabled_page.html (right): > > https://codereview.chromium.org/273103003/diff/20001/tools/telemetry/unittest... > tools/telemetry/unittest_data/interaction_enabled_page.html:57: <script> > Should this be in <body>? Done.
Nat's definitely a better reviewer than me for this.
lgtm This looks fine to me, but please wait for Nat's review. https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:7: from telemetry.web_perf import timeline_interaction_record as tir_module OOC, why do we rename this to tir_module? I think timeline_interaction_record is fine as a name. (Also, you can do import telemetry.web_perf.timeline_interaction_record as tir_module I think.) https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:17: def IsTopSlice(event): Private? https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:82: class MainthreadJankMetric(timeline_based_metric.TimelineBasedMetric): Do you want to put some documentation here? https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:112: results.Add('num_big_janks', 'score', num_big_janks) num_big_mainthread_jank instead (as opposed to jank metrics in smoothness)? Same below. It does feel a bit clumsy to say total_mainthread_jank_thread_time, but I can't think of better wording.
https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:7: from telemetry.web_perf import timeline_interaction_record as tir_module On 2014/05/29 21:20:10, chrishenry wrote: > OOC, why do we rename this to tir_module? I think timeline_interaction_record is > fine as a name. (Also, you can do import > telemetry.web_perf.timeline_interaction_record as tir_module I think.) timeline_interaction_record is quite long, so it may create some unnecessarily awkward formatting situation. The style of "from ... import ..." is used everywhere in telemetry, so I prefer it to be consistent. https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:17: def IsTopSlice(event): On 2014/05/29 21:20:10, chrishenry wrote: > Private? Done. https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:82: class MainthreadJankMetric(timeline_based_metric.TimelineBasedMetric): On 2014/05/29 21:20:10, chrishenry wrote: > Do you want to put some documentation here? Done. https://codereview.chromium.org/273103003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:112: results.Add('num_big_janks', 'score', num_big_janks) On 2014/05/29 21:20:10, chrishenry wrote: > num_big_mainthread_jank instead (as opposed to jank metrics in smoothness)? Same > below. > > It does feel a bit clumsy to say total_mainthread_jank_thread_time, but I can't > think of better wording. Done.
some feedback https://codereview.chromium.org/273103003/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): https://codereview.chromium.org/273103003/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:70: if not _IsTopSlice(s): so we have "toplevel slices" on a thread that are the ones at the top of the nesting stack. please make iterallslices use that array instead of using this istopslice thing and making assumptions about trace event names. some tasks dont have messageloop as the top and its a fragile assumption https://codereview.chromium.org/273103003/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:129: 'Main thread jank metrics cannot be computed since trace does not ' this is going to spam the console hugely... can you come up with a solution that prints once? https://codereview.chromium.org/273103003/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:133: results.Add('num_big_mainthread_janks', 'score', num_big_janks) are these all important? can you please pick 1 that is important and pass 'unimportant' as the data type for the others?
https://codereview.chromium.org/273103003/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py (right): https://codereview.chromium.org/273103003/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:70: if not _IsTopSlice(s): On 2014/05/29 23:33:12, nduca wrote: > so we have "toplevel slices" on a thread that are the ones at the top of the > nesting stack. please make iterallslices use that array instead of using this > istopslice thing and making assumptions about trace event names. some tasks dont > have messageloop as the top and its a fragile assumption Done. I just iterate through all the top slices. If they don't overlap, jank_thread_duration is 0 and it works fine for the metrics. In term of run time performance, it's always O(n) even if we can use binary search to get the start & end, and I believe the number of top_slices is not that big. FYI, I still keep the IterAllSlicesOverlappedWithTimeRange in thread.py since I think all timeline based metrics should handle the corner case of overlaps on the left & right. https://codereview.chromium.org/273103003/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:129: 'Main thread jank metrics cannot be computed since trace does not ' On 2014/05/29 23:33:12, nduca wrote: > this is going to spam the console hugely... can you come up with a solution that > prints once? By hugely, do you mean one log line per list of records? I think it wouldn't be too bad since with timeline_based_measurement, this maps to the number of records on the timeline, and we already get stdout result for each record with timeline_based_measurement anyway. In addition, I just add some more helpful debugging information. https://codereview.chromium.org/273103003/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/metrics/mainthread_jank.py:133: results.Add('num_big_mainthread_janks', 'score', num_big_janks) On 2014/05/29 23:33:12, nduca wrote: > are these all important? can you please pick 1 that is important and pass > 'unimportant' as the data type for the others? I think num_big_mainthread_janks and total_big_jank_thread_time are both important. The former tells us how many times users get the annoying perceivable delay, whereas the latter tells us the total amount of time that users are "unhappy". wdyt?
Please pull off your edits to timeline and iter to a new patch so i can review this more efficiently. tbr those edits to me. I'd like you to pick one metric, two at most. I will *not* lg this with more than that number, not initially. Eventually, we can add more, even as a followup. But I'd ask you to find out the minimal thing you can do to detect that there has been a regression. Any "diagnostic" metrics can be computed in about:tracing. Start with a simple number, lay down infrastructure. Then add to it. When you do the other route, you have an inferior result. I believe total_jank_over_thresshold or biggest_jank is sufficient for monitoring purposes. especially when we combine it with ari's queueing metrics and we implement the is_fast metrics. This is all the more reason to do just one. Can you come up with a name for the metrics reported so that they're prefixed with something that makes it obvious that they came from the is_responsive flag? responsiveness.biggest_jank for instance. We could always come back and tweak the names of the smoothness metrics [which are also too numerous, btw].
On 2014/06/03 00:12:58, nduca wrote: > Please pull off your edits to timeline and iter to a new patch so i can review > this more efficiently. tbr those edits to me. > > I'd like you to pick one metric, two at most. I will *not* lg this with more > than that number, not initially. Eventually, we can add more, even as a > followup. But I'd ask you to find out the minimal thing you can do to detect > that there has been a regression. Any "diagnostic" metrics can be computed in > about:tracing. Start with a simple number, lay down infrastructure. Then add to > it. When you do the other route, you have an inferior result. > > I believe total_jank_over_thresshold or biggest_jank is sufficient for > monitoring purposes. especially when we combine it with ari's queueing metrics > and we implement the is_fast metrics. This is all the more reason to do just > one. Agree. > Can you come up with a name for the metrics reported so that they're prefixed > with something that makes it obvious that they came from the is_responsive flag? > responsiveness.biggest_jank for instance. We could always come back and tweak > the names of the smoothness metrics [which are also too numerous, btw]. Done. Pulled the computation of the mainthread_jank to mainthread_jank_stats and renamed mainthread_jank to responsiveness_metric.
lgtm https://codereview.chromium.org/273103003/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/responsiveness_metric.py (right): https://codereview.chromium.org/273103003/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/responsiveness_metric.py:37: logging.warning( please turn this to TODO(nednguyen): Report a warning and do a followup patch to the page_results system where you can report in a warning such that the results object handles supressing all but the first instance of that warning. i dont want 50 lines of logspew if i run this on a windows machine, nor do i want an ad hoc solution here for that problem.
https://codereview.chromium.org/273103003/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/metrics/responsiveness_metric.py (right): https://codereview.chromium.org/273103003/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/metrics/responsiveness_metric.py:37: logging.warning( On 2014/06/04 18:51:22, nduca wrote: > please turn this to > > TODO(nednguyen): Report a warning > > and do a followup patch to the page_results system where you can report in a > warning such that the results object handles supressing all but the first > instance of that warning. i dont want 50 lines of logspew if i run this on a > windows machine, nor do i want an ad hoc solution here for that problem. Done.
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nednguyen@google.com/273103003/120001
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nednguyen@google.com/273103003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nednguyen@google.com/273103003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nednguyen@google.com/273103003/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
Message was sent while issue was closed.
Change committed as 275036 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
