Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
636203002:
Task Execution Duration metrics added as a new benchmark (Closed)
DescriptionTo help guide development and track performance improvements in specific areas this new metric will compile the top-10 slowest tasks across all the pages in a page-set.
Part of the scheduling effort is to move long-running tasks into 'idle' periods or refactor them to make them more nibble-able. Knowing which tasks we need to concentrate on is the first step in this.
It is hoped that this code will also be built-on to create a metric to find out which long-running tasks most frequently block high priority tasks (e.g. Does GC often block user input being processed?) so we can move them out of the way.
BUG=421434
Committed: https://crrev.com/1a103b57e6ff05274635c8a89e40bfaa6000a9ca
Cr-Commit-Position: refs/heads/master@{#300084}
Patch Set 1 #
Total comments: 66
Patch Set 2 : White space removed, copyright fixed, code python-ised #Patch Set 3 : Implementing code review feedback #Patch Set 4 : Small code layout refactor to aid readability #
Total comments: 4
Patch Set 5 : Individual page data no longer removed #
Total comments: 12
Patch Set 6 : Unit test added #
Total comments: 10
Patch Set 7 : Nits fixed in unit test #
Total comments: 5
Patch Set 8 : Layout fixes #
Total comments: 4
Patch Set 9 : White space fix + removal of debug code #
Messages
Total messages: 28 (3 generated)
I've added a new benchmark 'task_execution_time' to report on the top 10 slowest tasks. It re-uses the ScrollAction from the smoothness tests to give the page something to do. I've enabled it for key_mobile_sites and tough_scheduling_cases. In order to keep the results easily sortable by duration when run with multiple pages I have added code to only show summaries (i.e. the bold entries, without the per-page results). I've plumbed this flag down into summary.py using the skip class; I don't know if there is a better way to do this? I also don't yet know if this will cause the dashboard display to complain (I'm looking into this next).
alexclarke@google.com changed reviewers: + alexclarke@google.com
https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... File tools/perf/benchmarks/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... tools/perf/benchmarks/task_execution_time.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. Is that the right copyright message? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:30: nit: I assume we don't want blank lines after a function definition?
https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... File tools/perf/benchmarks/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... tools/perf/benchmarks/task_execution_time.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. I copied it from smoothness.py, not sure if it should be 2014 though? Does anyone know? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:30: Yes, you're right. My eyes prefer it with spaces, but the other files don't do this, so I'll remove them.
https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... File tools/perf/benchmarks/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... tools/perf/benchmarks/task_execution_time.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. The header should contain the year in which the file was added (see http://dev.chromium.org/developers/coding-style#TOC-File-headers). https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. Again should be "2014". https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:43: def DidRunActions(self, page, tab): Is there any reason for double blank lines around this method? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:63: while count < max_tasks: "for count in xrange(max_tasks):" is more suited for this. An even more elegant solution would be "for task in sorted_tasks[:NUMBER_OF_RESULTS_TO_DISPLAY]:". https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:64: task = sorted_tasks.pop(0) Popping from the beginning of a list is quite costly. Why not "task = sorted_tasks[count]"? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:79: depth = 1 depth is a constant (you don't change it anywhere in this method). Perhaps the corresponding argument of ProcessTasksForSlice should be the last one and have a default value 1. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:81: for task_slice in process.IterAllSlicesOfName('MessageLoop::RunTask'): for parent, task_slice in enumerate(process.IterAllSlicesOfName('MessageLoop::RunTask')): https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:86: tasks = [sliceData for key, sliceData in task_dictionary.items()] return task_dictionary.values() https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:98: self.sum_self_duration = self_duration Shouldn't there be some systematic order (e.g. sum, sum, min, min, max, max)? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:111: self.count = self.count + 1 self.count += 1 https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:113: self.sum_self_duration = (self.sum_self_duration + self_duration) Again, I would suggest using the += operator instead and imposing an order. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:121: self.tree_location.append( (depth, parent) ) Extra whitespace around the tuple. https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... File tools/telemetry/telemetry/value/summary.py (right): https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/summary.py:138: num_successful_pages_for_this_value_name) Invalid indentation
Thanks Simon, some comments below. https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... File tools/perf/benchmarks/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... tools/perf/benchmarks/task_execution_time.py:12: """Measures rendering statistics while scrolling down the key mobile sites. Should we say task execution statistics instead of rendering statistics? https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... tools/perf/benchmarks/task_execution_time.py:19: """Measures rendering statistics while scrolling the tough scheduling sites. Ditto. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. Ditto about the copyright year. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:12: CATEGORIES = ['webkit.console', Assuming these constants aren't meant to be public, prepend an underscore to their names. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:24: I don't think we generally have blank lines between function declarations and bodies, so I'd follow the existing style here. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:26: super(TaskExecutionTime, self).__init__('RunTaskExecutionTime') Why not reuse RunSmoothness for this? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:40: options, category_filter, TIME_OUT_IN_SECONDS) indent +2 https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:55: key = lambda slice: slice.meanSelfDuration, reverse=True) nit: No spaces around "=" for named parameters. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:59: results.AddValue(skip.SkipValue.SetOnlyShowingSummaries(page)) Instead of this trick, could we just report the summary values ourselves? You can pass in None to ScalarValue to indicate that it's not associated with a single page. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:63: while count < max_tasks: nit: could also be written as: for task in sorted_tasks[:NUMBER_OF_RESULTS_TO_DISPLAY]: [...] https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:68: "ms", ' instead of " https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:70: description="Slowest tasks")) ' instead of " https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:74: def GetTasks(self, process): Does this need to be a member or could it be a free function? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:86: tasks = [sliceData for key, sliceData in task_dictionary.items()] nit: Same as writing task_dictionary.values() https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:121: self.tree_location.append( (depth, parent) ) nit: no spaces outside the parens. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:124: def meanSelfDuration(self): mean_self_duration. Do we want mean or median? The latter could be less prone to outliers. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:126: if self.count == 0: If count starts at 1, does this ever happen? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:150: ProcessTasksForSlice(dictionary, sub_slice, depth+1, parent) nit: add spaces around the '+' https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... File tools/telemetry/telemetry/value/skip.py (right): https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/skip.py:7: ONLY_SUMMARIES = "Only Showing Summaries" Prepend and underscore here to keep this private. Also use single quotes instead of double quotes. https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/skip.py:66: def onlySummaries(self): only_summaries https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... File tools/telemetry/telemetry/value/summary.py (right): https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/summary.py:47: for skip_me in all_page_specific_values) nit: the wrapping here is misleading since the second line isn't inside the call to isinstance. https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/summary.py:138: num_successful_pages_for_this_value_name) Accidental change?
Code review fixes. https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... File tools/perf/benchmarks/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... tools/perf/benchmarks/task_execution_time.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2014/10/08 14:01:48, petrcermak wrote: > The header should contain the year in which the file was added (see > http://dev.chromium.org/developers/coding-style#TOC-File-headers). Done. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:43: def DidRunActions(self, page, tab): I've removed the blank lines after the function definition now. I'm putting double blanks lines between all function definitions, which is wrong :) I'll fix it. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:63: while count < max_tasks: Nice... Done! https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:64: task = sorted_tasks.pop(0) Not needed any more with your earlier suggestion. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:79: depth = 1 I made it a constant to give it a name, rather than passing a naked '1' into the function. I'm not keen on default values as you need to seek out the function definition to know what it' doing. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:86: tasks = [sliceData for key, sliceData in task_dictionary.items()] On 2014/10/08 14:01:48, petrcermak wrote: > return task_dictionary.values() Done. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:98: self.sum_self_duration = self_duration Good call. I've re-ordered them. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:113: self.sum_self_duration = (self.sum_self_duration + self_duration) On 2014/10/08 14:01:48, petrcermak wrote: > Again, I would suggest using the += operator instead and imposing an order. Done. https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... File tools/telemetry/telemetry/value/summary.py (right): https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/summary.py:138: num_successful_pages_for_this_value_name) On 2014/10/08 14:01:48, petrcermak wrote: > Invalid indentation Done.
Fixes based on Sami's review https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... File tools/perf/benchmarks/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/benchmarks/task_e... tools/perf/benchmarks/task_execution_time.py:12: """Measures rendering statistics while scrolling down the key mobile sites. Yes! Ctrl-C Ctrl-V error... https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:12: CATEGORIES = ['webkit.console', On 2014/10/08 14:51:52, Sami wrote: > Assuming these constants aren't meant to be public, prepend an underscore to > their names. Done. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:26: super(TaskExecutionTime, self).__init__('RunTaskExecutionTime') I guess these are currently the same; I made a separate version so we could diverge if needed. Maybe a case of YAGNI? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:55: key = lambda slice: slice.meanSelfDuration, reverse=True) On 2014/10/08 14:51:53, Sami wrote: > nit: No spaces around "=" for named parameters. Done. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:59: results.AddValue(skip.SkipValue.SetOnlyShowingSummaries(page)) We can't accumulate values between separate executions on different pages (e.g. Garbage collection on Bing + TheVerge); this is done at a higher level by the code in summary.py. Passing None as a page makes the code fall over, did I misunderstand your suggestion? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:63: while count < max_tasks: Petr made this observation too. Done. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:68: "ms", On 2014/10/08 14:51:53, Sami wrote: > ' instead of " Done. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:70: description="Slowest tasks")) On 2014/10/08 14:51:53, Sami wrote: > ' instead of " Done. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:74: def GetTasks(self, process): It could be free. Is it not best to encapsulate it (to the extent that anything is encapsulated in Python)? https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:124: def meanSelfDuration(self): Done https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:126: if self.count == 0: Ha! You are correct... old habits die hard! This is gone with the change to Median anyway. https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... File tools/telemetry/telemetry/value/skip.py (right): https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/skip.py:7: ONLY_SUMMARIES = "Only Showing Summaries" On 2014/10/08 14:51:53, Sami wrote: > Prepend and underscore here to keep this private. Also use single quotes instead > of double quotes. Done. https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/skip.py:66: def onlySummaries(self): On 2014/10/08 14:51:53, Sami wrote: > only_summaries Done. https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... File tools/telemetry/telemetry/value/summary.py (right): https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/summary.py:47: for skip_me in all_page_specific_values) Oops. Missed this comment. Will fix on next round of updates.
Maybe flesh out the description a bit with a motivating detail or two about the metric?
Small layout fix based on Sami's code review. https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... File tools/telemetry/telemetry/value/summary.py (right): https://codereview.chromium.org/636203002/diff/1/tools/telemetry/telemetry/va... tools/telemetry/telemetry/value/summary.py:47: for skip_me in all_page_specific_values) On 2014/10/08 15:58:27, picksi1 wrote: > Oops. Missed this comment. Will fix on next round of updates. Done.
https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:59: results.AddValue(skip.SkipValue.SetOnlyShowingSummaries(page)) On 2014/10/08 15:58:26, picksi1 wrote: > We can't accumulate values between separate executions on different pages (e.g. > Garbage collection on Bing + TheVerge); this is done at a higher level by the > code in summary.py. > > Passing None as a page makes the code fall over, did I misunderstand your > suggestion? The documentation for Value suggests that the page could be None: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... Is there some postprocessor hook we could use to make the results look like what we want? This skip value seems a little odd to me. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:74: def GetTasks(self, process): On 2014/10/08 15:58:26, picksi1 wrote: > It could be free. Is it not best to encapsulate it (to the extent that anything > is encapsulated in Python)? Encapsulation is fine, but it could still be a @classmethod or a @staticmethod. https://codereview.chromium.org/636203002/diff/60001/tools/perf/measurements/... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/60001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:25: Extra blank line here. https://codereview.chromium.org/636203002/diff/60001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:103: def medianSelfDuration(self): hacker_style instead of camelCase please.
I'm no longer removing individual page data. I'll fix up the data viewer to be more user friendly and hide the data in the UI rather than deleting it at source. https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/1/tools/perf/measurements/task... tools/perf/measurements/task_execution_time.py:59: results.AddValue(skip.SkipValue.SetOnlyShowingSummaries(page)) As discussed offline, I'll look into a way to change the UI of the benchmark viewer and leave the data alone. https://codereview.chromium.org/636203002/diff/60001/tools/perf/measurements/... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/60001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:25: On 2014/10/09 10:28:14, Sami wrote: > Extra blank line here. Done. https://codereview.chromium.org/636203002/diff/60001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:103: def medianSelfDuration(self): On 2014/10/09 10:28:14, Sami wrote: > hacker_style instead of camelCase please. Done.
Could you also add a unit test for the new measurement? https://codereview.chromium.org/636203002/diff/80001/tools/perf/benchmarks/ta... File tools/perf/benchmarks/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/80001/tools/perf/benchmarks/ta... tools/perf/benchmarks/task_execution_time.py:8: nit: two spaces between top level items. https://codereview.chromium.org/636203002/diff/80001/tools/perf/benchmarks/ta... tools/perf/benchmarks/task_execution_time.py:15: ditto https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:49: key = lambda slice: slice.median_self_duration, reverse = True) No spaces around '=' here. https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:68: ditto https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:100: ditto
+cc vmiura@ who might have some insights into measuring/retrieving task executing durations. https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:26: self._renderer_thread = None It's kind of a shame to expose only renderer thread events, as this could prove particularly useful for identifiying compositor/ui/io thread bottlenecks. Could we identify the top N per major thread? Or would adding the additional necessary categories introduce too much noise? I'd be OK with that happening in a follow-up patch if we're simply focused on getting this working for the renderer thread.
The renderer-thread is my whole world at the moment! I'm happy to expand this focus in later CLs, but I'd like to keep this renderer thread focused for now.
I've added two unit tests (1) confirm that we get back the number of results expected (2) the results are in sorted order [as a proxy for determining that we're getting the top N tasks]. https://codereview.chromium.org/636203002/diff/80001/tools/perf/benchmarks/ta... File tools/perf/benchmarks/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/80001/tools/perf/benchmarks/ta... tools/perf/benchmarks/task_execution_time.py:8: On 2014/10/09 13:14:46, Sami wrote: > nit: two spaces between top level items. Done. https://codereview.chromium.org/636203002/diff/80001/tools/perf/benchmarks/ta... tools/perf/benchmarks/task_execution_time.py:15: On 2014/10/09 13:14:46, Sami wrote: > ditto Done. https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:26: self._renderer_thread = None Let's look at expanding the scope later. https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:49: key = lambda slice: slice.median_self_duration, reverse = True) On 2014/10/09 13:14:47, Sami wrote: > No spaces around '=' here. Done. https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:68: On 2014/10/09 13:14:46, Sami wrote: > ditto Done. https://codereview.chromium.org/636203002/diff/80001/tools/perf/measurements/... tools/perf/measurements/task_execution_time.py:100: On 2014/10/09 13:14:46, Sami wrote: > ditto Done.
Thanks for adding the test! https://codereview.chromium.org/636203002/diff/120001/tools/perf/measurements... File tools/perf/measurements/task_execution_time_unittest.py (right): https://codereview.chromium.org/636203002/diff/120001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:29: """ Looks like this would fit on the previous line. The comment sounds a bit redundant however. https://codereview.chromium.org/636203002/diff/120001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:42: self.assertEquals( nit pick: What if there are less than 10 tasks in total? :) https://codereview.chromium.org/636203002/diff/120001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:56: results.all_page_specific_values[index+1].value) nit: spaces around the plus. A fancier way to do this, but what you have works too: for first, second in zip(results.all_page_specific_values, results.all_page_specific_values[1:]): self.assertGreaterEqual(first.value, second.value) https://codereview.chromium.org/636203002/diff/120001/tools/perf/page_sets/ke... File tools/perf/page_sets/key_mobile_sites.py (right): https://codereview.chromium.org/636203002/diff/120001/tools/perf/page_sets/ke... tools/perf/page_sets/key_mobile_sites.py:13: credentials_path = 'data/credentials.json') No spaces around the '='. https://codereview.chromium.org/636203002/diff/120001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_scheduling_cases.py (right): https://codereview.chromium.org/636203002/diff/120001/tools/perf/page_sets/to... tools/perf/page_sets/tough_scheduling_cases.py:12: url=url, page_set=page_set, credentials_path = 'data/credentials.json') Ditto.
Some fixes from code review. Not sure if I should add hand-created data to fix the < 10 tasks issue? Does it merit the effort given it is working OK on a test page that (probably) won't change? https://codereview.chromium.org/636203002/diff/120001/tools/perf/measurements... File tools/perf/measurements/task_execution_time_unittest.py (right): https://codereview.chromium.org/636203002/diff/120001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:29: """ Removed this comment as it is redundant. https://codereview.chromium.org/636203002/diff/120001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:42: self.assertEquals( Hmmm. I did consider hand-creating some event data to pass into the code, but this felt like over-kill when the existing test page seems to work OK. I'm not sure how much time to spend this? https://codereview.chromium.org/636203002/diff/120001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:56: results.all_page_specific_values[index+1].value) Nice. Petr, Simon and myself discussed the various ways of doing this. Your option is neatest! https://codereview.chromium.org/636203002/diff/120001/tools/perf/page_sets/ke... File tools/perf/page_sets/key_mobile_sites.py (right): https://codereview.chromium.org/636203002/diff/120001/tools/perf/page_sets/ke... tools/perf/page_sets/key_mobile_sites.py:13: credentials_path = 'data/credentials.json') On 2014/10/10 17:41:43, Sami wrote: > No spaces around the '='. Done. https://codereview.chromium.org/636203002/diff/120001/tools/perf/page_sets/to... File tools/perf/page_sets/tough_scheduling_cases.py (right): https://codereview.chromium.org/636203002/diff/120001/tools/perf/page_sets/to... tools/perf/page_sets/tough_scheduling_cases.py:12: url=url, page_set=page_set, credentials_path = 'data/credentials.json') On 2014/10/10 17:41:43, Sami wrote: > Ditto. Done.
Some fixes from code review. Not sure if I should add hand-created data to fix the < 10 tasks issue? Does it merit the effort given it is working OK on a test page that (probably) won't change?
https://codereview.chromium.org/636203002/diff/260001/tools/perf/measurements... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/260001/tools/perf/measurements... tools/perf/measurements/task_execution_time.py:65: process.IterAllSlicesOfName('MessageLoop::RunTask')): This line needs to be indented 4 spaces with respect to the for loop header. Otherwise, it is difficult to distinguish it from the loop body. See http://legacy.python.org/dev/peps/pep-0008/#indentation (note that we use two-space indentation though http://dev.chromium.org/developers/coding-style#TOC-Python). Also note that this does not apply to the next line (66). https://codereview.chromium.org/636203002/diff/260001/tools/perf/measurements... File tools/perf/measurements/task_execution_time_unittest.py (right): https://codereview.chromium.org/636203002/diff/260001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:14: page_set, base_dir) The indentation is not in line with the style guide (http://legacy.python.org/dev/peps/pep-0008/). Namely, either page_set must be vertically aligned with 'file://...', or 'file://...' must already be on a new line. https://codereview.chromium.org/636203002/diff/260001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:51: results.all_page_specific_values[1:]): ditto.
Python layout issues fixed. PTAL. https://codereview.chromium.org/636203002/diff/260001/tools/perf/measurements... File tools/perf/measurements/task_execution_time.py (right): https://codereview.chromium.org/636203002/diff/260001/tools/perf/measurements... tools/perf/measurements/task_execution_time.py:65: process.IterAllSlicesOfName('MessageLoop::RunTask')): On 2014/10/13 16:46:23, petrcermak wrote: > This line needs to be indented 4 spaces with respect to the for loop header. > Otherwise, it is difficult to distinguish it from the loop body. See > http://legacy.python.org/dev/peps/pep-0008/#indentation (note that we use > two-space indentation though > http://dev.chromium.org/developers/coding-style#TOC-Python). Also note that this > does not apply to the next line (66). Done. https://codereview.chromium.org/636203002/diff/260001/tools/perf/measurements... File tools/perf/measurements/task_execution_time_unittest.py (right): https://codereview.chromium.org/636203002/diff/260001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:51: results.all_page_specific_values[1:]): On 2014/10/13 16:46:23, petrcermak wrote: > ditto. Done.
Thanks, lgtm with a couple of nits. https://codereview.chromium.org/636203002/diff/330001/tools/perf/measurements... File tools/perf/measurements/task_execution_time_unittest.py (right): https://codereview.chromium.org/636203002/diff/330001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:10: nit: one more blank line here. https://codereview.chromium.org/636203002/diff/330001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:23: def dump(obj): This isn't used anywhere, is it?
https://codereview.chromium.org/636203002/diff/330001/tools/perf/measurements... File tools/perf/measurements/task_execution_time_unittest.py (right): https://codereview.chromium.org/636203002/diff/330001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:10: On 2014/10/16 10:20:30, Sami wrote: > nit: one more blank line here. Done. https://codereview.chromium.org/636203002/diff/330001/tools/perf/measurements... tools/perf/measurements/task_execution_time_unittest.py:23: def dump(obj): Oops. Well spotted, that's just debug code.
The CQ bit was checked by picksi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/636203002/350001
Message was sent while issue was closed.
Committed patchset #9 (id:350001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1a103b57e6ff05274635c8a89e40bfaa6000a9ca Cr-Commit-Position: refs/heads/master@{#300084} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
