Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18261009:
Have repeats understand "time" (Closed)
|
Created:
7 years, 5 months ago by edmundyan Modified:
7 years, 5 months ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org, Mike Stip (use stip instead) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description--page-repeat and --pageset-repeat can now be set to repeat for an amount of time by appending an 's' after the integer
WillRunPageSet has been renamed to WillRunTest
DidRunPageSet has been renamed to DidRunTest
BUG=260008
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213184
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213360
Patch Set 1 #
Total comments: 19
Patch Set 2 : . #
Total comments: 24
Patch Set 3 : Adding regex for arg parsing and suggested changes #
Total comments: 13
Patch Set 4 : Changes from dtu. #
Total comments: 3
Patch Set 5 : Merging time and iters into a single loop and allowing combinations for page/pagesets #
Total comments: 2
Patch Set 6 : . #
Total comments: 14
Patch Set 7 : Changes from dennisjeffrey #
Total comments: 1
Patch Set 8 : Fixing conditionals on repeat loop. Moving check CanRunForPage() earlier #
Total comments: 6
Patch Set 9 : Refactoring out repeat logic/runner into separate classes #
Total comments: 2
Patch Set 10 : Removing option.Options class. Clarification from nduca #
Total comments: 24
Patch Set 11 : Fixing nits. Moving repeat logic back to Run() #Patch Set 12 : Rebasing.. Also found a bug in StartProling() and created IsRepeating() function #
Total comments: 6
Patch Set 13 : . #Patch Set 14 : Override deepcopy in RepeatOptions #Patch Set 15 : . #Patch Set 16 : Rebasing. Fixing unittests #Patch Set 17 : Fixing perf tests #Messages
Total messages: 61 (0 generated)
Would like a peek to see if I'm on the right track here, thanks! page_runner.py is pretty messy but it's mostly just refactoring the inner loop into another function
Great! Some initial high-level comments. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/browser_options.py:130: group.add_option('--repeat-page', dest='repeat_page', default=0, maybe this should appear above, right underneath --page-repeat, since it's almost the same thing, except it repeats for a time rather than for a count. Similar comment for --repeat-pageset below. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/browser_options.py:130: group.add_option('--repeat-page', dest='repeat_page', default=0, I think we should also probably change the name of this option. We have one called --page-repeat and another called --repeat-page, and it's not clear which one is for a count and which one is for a time. How about: "--page-repeat-time" and "--pageset-repeat-time"? https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:170: def _RunInnerLoop(test, page_set, options, page, credentials_path, it would be good to name this with something a bit more descriptive. Since it looks like this is running a single page, how about something like "_RunPage"? https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:225: results_for_current_run.StopTest(page) one thing we'll have to keep in mind is related to task #4 on the design doc: teach the Telemetry results object to understand results that may happen from repeats. It would likely be best to implement that in a separate CL, so for the current CL, we just need to make sure that the system is doing something reasonable when it's reporting results in the case when we repeat for some length of time. What do the results look like in the current CL when we repeat either a single page or a pageset for some length of time? Does it report all results from each iteration that is run during that time, or does it do something else? https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:259: test.DidRunPageSet(state.tab, results) Regarding your TODO: I believe this was previously only invoked as soon as we're done running everything, so for now let's keep it like it is (only after the loop for each pageset, not within it).
https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/browser_options.py:130: group.add_option('--repeat-page', dest='repeat_page', default=0, On 2013/07/08 23:33:08, dennis_jeffrey wrote: > I think we should also probably change the name of this option. We have one > called --page-repeat and another called --repeat-page, and it's not clear which > one is for a count and which one is for a time. How about: "--page-repeat-time" > and "--pageset-repeat-time"? How about if you reuse the existing options to do something like --pageset-repeat=10 for 10 iterations and --pageset-repeat=10s for 10 seconds? https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:251: pageset_time += results.duration The timing for results isn't really well-defined. It might or might not include things like launching the browser and other setup or teardown. Maybe you should explicitly time it here instead. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:259: test.DidRunPageSet(state.tab, results) On 2013/07/08 23:33:08, dennis_jeffrey wrote: > Regarding your TODO: I believe this was previously only invoked as soon as we're > done running everything, so for now let's keep it like it is (only after the > loop for each pageset, not within it). Yes, it seems like it should be called after all pageset iterations are finished. (I don't think it _really_ matters, though.) Since that's the case, you should rename it to something more like DidRunTest(). Also, is there any way to move WillRunPageSet() near here to match the DidRun()?
Some clarifying questions before I work on the change. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/browser_options.py:130: group.add_option('--repeat-page', dest='repeat_page', default=0, My change disallows mixing time/iteration options, but that was mainly for when setting '--page-repeat' and '--repeat-page' at the same time and causing ambiguity. Adding a 's' suffix would be a nice fix to this, but opens up the ability of mixing time/iterations for pages and pagesets - is this okay? https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:225: results_for_current_run.StopTest(page) There is no distinction between results from time versus from iterations. It will report all results from each iteration during the time it was run. Each measurement will be an array of floats for that specific URL. buildbot output: RESULT mean_frame_time_by_url: http___www.amazon.com= [8.814,8.628,8.458,8.2] ms Avg mean_frame_time_by_url: 8.525000ms Sd mean_frame_time_by_url: 0.260924ms This could be the results for running the page for four iterations, the page for 16 seconds, or other combinations. I think the results reporting for length-of-time is reasonable for now as it is in line with that iteration reporting is doing. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:259: test.DidRunPageSet(state.tab, results) Should there be any backward compatibility from DidRunPageSet() to DidRunTest()? Or should I update the scripts in "src/tools/perf/perf_tools/" + any others that use the old DidRunPageSet in this CL? Same for changing WillRunPageSet() to WillRunTest()
https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/browser_options.py:130: group.add_option('--repeat-page', dest='repeat_page', default=0, On 2013/07/09 01:42:47, edmundyan wrote: > My change disallows mixing time/iteration options, but that was mainly for when > setting '--page-repeat' and '--repeat-page' at the same time and causing > ambiguity. Adding a 's' suffix would be a nice fix to this, but opens up the > ability of mixing time/iterations for pages and pagesets - is this okay? If we want to still disallow mixing the time/iteration options, we could still enforce that: if both --page-repeat and --pageset-repeat are specified, then either both must have an 's', or both must not. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:225: results_for_current_run.StopTest(page) On 2013/07/09 01:42:47, edmundyan wrote: > There is no distinction between results from time versus from iterations. It > will report all results from each iteration during the time it was run. Each > measurement will be an array of floats for that specific URL. > > buildbot output: > RESULT mean_frame_time_by_url: http___www.amazon.com= [8.814,8.628,8.458,8.2] ms > Avg mean_frame_time_by_url: 8.525000ms > Sd mean_frame_time_by_url: 0.260924ms > > This could be the results for running the page for four iterations, the page for > 16 seconds, or other combinations. > > I think the results reporting for length-of-time is reasonable for now as it is > in line with that iteration reporting is doing. Sounds good to me. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:259: test.DidRunPageSet(state.tab, results) On 2013/07/09 01:42:47, edmundyan wrote: > Should there be any backward compatibility from DidRunPageSet() to DidRunTest()? > Or should I update the scripts in "src/tools/perf/perf_tools/" + any others > that use the old DidRunPageSet in this CL? Same for changing WillRunPageSet() > to WillRunTest() I think that if we change the function name, it's best to change all call sites for that function to use the new name (if possible). Supporting both the old and new function names adds unnecessary complexity to the codebase and probably should be avoided if we can help it.
https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:259: test.DidRunPageSet(state.tab, results) On 2013/07/09 01:42:47, edmundyan wrote: > Should there be any backward compatibility from DidRunPageSet() to DidRunTest()? > Or should I update the scripts in "src/tools/perf/perf_tools/" + any others > that use the old DidRunPageSet in this CL? Same for changing WillRunPageSet() > to WillRunTest() Rename them to DidRunTest(). There's only 2 users of DidRunPageSet(), and 0 users of WillRunPageSet(), so this is a safe change.
Made changes and uploaded a new patch. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/browser_options.py:130: group.add_option('--repeat-page', dest='repeat_page', default=0, On 2013/07/09 17:27:52, dennis_jeffrey wrote: > On 2013/07/09 01:42:47, edmundyan wrote: > > My change disallows mixing time/iteration options, but that was mainly for > when > > setting '--page-repeat' and '--repeat-page' at the same time and causing > > ambiguity. Adding a 's' suffix would be a nice fix to this, but opens up the > > ability of mixing time/iterations for pages and pagesets - is this okay? > > If we want to still disallow mixing the time/iteration options, we could still > enforce that: if both --page-repeat and --pageset-repeat are specified, then > either both must have an 's', or both must not. Done. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:170: def _RunInnerLoop(test, page_set, options, page, credentials_path, On 2013/07/08 23:33:08, dennis_jeffrey wrote: > it would be good to name this with something a bit more descriptive. Since it > looks like this is running a single page, how about something like "_RunPage"? Done. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:251: pageset_time += results.duration On 2013/07/08 23:56:08, Dave Tu wrote: > The timing for results isn't really well-defined. It might or might not include > things like launching the browser and other setup or teardown. Maybe you should > explicitly time it here instead. Done. https://codereview.chromium.org/18261009/diff/1/tools/telemetry/telemetry/pag... tools/telemetry/telemetry/page/page_runner.py:259: test.DidRunPageSet(state.tab, results) On 2013/07/09 17:27:52, dennis_jeffrey wrote: > On 2013/07/09 01:42:47, edmundyan wrote: > > Should there be any backward compatibility from DidRunPageSet() to > DidRunTest()? > > Or should I update the scripts in "src/tools/perf/perf_tools/" + any others > > that use the old DidRunPageSet in this CL? Same for changing WillRunPageSet() > > to WillRunTest() > > I think that if we change the function name, it's best to change all call sites > for that function to use the new name (if possible). Supporting both the old > and new function names adds unnecessary complexity to the codebase and probably > should be avoided if we can help it. Done.
https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:55: self.pageset_repeat_time = 0.0 maybe initialize the above 2 variables to "None" instead of 0.0, since they're checked in a condition below at line 234: self.page_repeat_time or self.pageset_repeat_time (I'm a little more comfortable using either "None" or "0" when checking in a condition, rather than a float). https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:124: 'an \'s\' to specify length of time in seconds') Maybe also give an example in the help string, something like this: e.g., '10' to repeat for 10 iterations, or '30s' to repeat for 30 seconds. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:225: if self.page_repeat != 1: # pylint: disable=E1101 is this check necessary? The condition at line 226 below being true would already imply that self.page_repeat != 1. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:226: if self.page_repeat[-1] == 's': maybe accept both upper and lowercase 's'? if self.page_repeat[-1] in ['s', 'S']: https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:229: if self.pageset_repeat != 1: # pylint: disable=E1101 similar comment as line 225 above https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:176: # TODO(eyan): maybe related to dtu's TODO, but results_for_current_run is put your full official LDAP here https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:177: # not used Are you sure it's unused? I thought this was an important object that stores results of a run? https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_test.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_test.py:116: browser is torn down.""" The first line should be a 1-line summary of the docstring. If you can't fit everything on 1 line, add a blank line, followed by the rest: """First line goes here. Anything else goes in paragraphs down here. """
Please also mention in the CL description that you're changing the name of the DidRunPageSet and WillRunPageSet functions.
https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:225: if self.page_repeat != 1: # pylint: disable=E1101 On 2013/07/10 20:11:20, dennis_jeffrey wrote: > is this check necessary? > > The condition at line 226 below being true would already imply that > self.page_repeat != 1. The default value is int(1), and would cause an error on the next line when we try to access it like a string. I could combine the nested ifs into one or use isinstance() to ensure it's a string? Also wondering if I need to add more checks here. "10secs" would pass this function without errors. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:177: # not used On 2013/07/10 20:11:20, dennis_jeffrey wrote: > Are you sure it's unused? I thought this was an important object that stores > results of a run? I mean the re-assigning to the variable 'results_for_current_run' is unnecessary. We should just modify 'results'
https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:127: 'pageset before finishing. Append an \'s\' to specify length of ' + Don't need the + signs here. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:225: if self.page_repeat != 1: # pylint: disable=E1101 On 2013/07/10 21:13:09, edmundyan wrote: > On 2013/07/10 20:11:20, dennis_jeffrey wrote: > > is this check necessary? > > > > The condition at line 226 below being true would already imply that > > self.page_repeat != 1. > The default value is int(1), and would cause an error on the next line when we > try to access it like a string. I could combine the nested ifs into one or use > isinstance() to ensure it's a string? > > Also wondering if I need to add more checks here. "10secs" would pass this > function without errors. I think you should keep 'page_repeat' as a string with a default value of '1'; and as you parse it, set 'page_repeat_seconds' or 'page_repeat_iterations' based on which one it is, and do the type conversion too, which will cause it to fail fast if it's the wrong type. Similar to how the 'extra_wpr_args_as_string' conversion to 'extra_wpr_args' is done, above. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:170: possible_browser, results, state): I'm glad you made this a separate method. The nesting level was getting kind of crazy :) How about _PrepareAndRunPage? https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:177: # not used On 2013/07/10 21:13:09, edmundyan wrote: > On 2013/07/10 20:11:20, dennis_jeffrey wrote: > > Are you sure it's unused? I thought this was an important object that stores > > results of a run? > I mean the re-assigning to the variable 'results_for_current_run' is > unnecessary. We should just modify 'results' Yeah, this is kind of hacky and the logic of it isn't obvious. (I take no responsibility for it :) The test expects a results object and adds measurement numbers to it. Some tests only want to record numbers for runs after the first (e.g. to test only with a warm cache). To do that, we just replace the "one true" results object with a dummy object, which is then discarded. Maybe you can add a comment here to make it clearer. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:257: break To do this more concisely: pageset_start_time = time.time() while time.time() - pageset_start_time < options.pageset_repeat_seconds: # Stuff.
https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:124: 'an \'s\' to specify length of time in seconds') On 2013/07/10 20:11:20, dennis_jeffrey wrote: > Maybe also give an example in the help string, something like this: > > e.g., '10' to repeat for 10 iterations, or '30s' to repeat for 30 seconds. Done. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:127: 'pageset before finishing. Append an \'s\' to specify length of ' + On 2013/07/10 23:15:41, Dave Tu wrote: > Don't need the + signs here. Done. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:225: if self.page_repeat != 1: # pylint: disable=E1101 On 2013/07/10 23:15:41, Dave Tu wrote: > On 2013/07/10 21:13:09, edmundyan wrote: > > On 2013/07/10 20:11:20, dennis_jeffrey wrote: > > > is this check necessary? > > > > > > The condition at line 226 below being true would already imply that > > > self.page_repeat != 1. > > The default value is int(1), and would cause an error on the next line when we > > try to access it like a string. I could combine the nested ifs into one or > use > > isinstance() to ensure it's a string? > > > > Also wondering if I need to add more checks here. "10secs" would pass this > > function without errors. > > I think you should keep 'page_repeat' as a string with a default value of '1'; > and as you parse it, set 'page_repeat_seconds' or 'page_repeat_iterations' based > on which one it is, and do the type conversion too, which will cause it to fail > fast if it's the wrong type. Similar to how the 'extra_wpr_args_as_string' > conversion to 'extra_wpr_args' is done, above. Done. Also using regex matching instead of character matching to add robustness (as per 1:1 w/ Dennis) https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:226: if self.page_repeat[-1] == 's': On 2013/07/10 20:11:20, dennis_jeffrey wrote: > maybe accept both upper and lowercase 's'? > > if self.page_repeat[-1] in ['s', 'S']: Done. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:170: possible_browser, results, state): On 2013/07/10 23:15:41, Dave Tu wrote: > I'm glad you made this a separate method. The nesting level was getting kind of > crazy :) > How about _PrepareAndRunPage? Done. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:177: # not used On 2013/07/10 23:15:41, Dave Tu wrote: > On 2013/07/10 21:13:09, edmundyan wrote: > > On 2013/07/10 20:11:20, dennis_jeffrey wrote: > > > Are you sure it's unused? I thought this was an important object that > stores > > > results of a run? > > I mean the re-assigning to the variable 'results_for_current_run' is > > unnecessary. We should just modify 'results' > > Yeah, this is kind of hacky and the logic of it isn't obvious. (I take no > responsibility for it :) The test expects a results object and adds measurement > numbers to it. Some tests only want to record numbers for runs after the first > (e.g. to test only with a warm cache). To do that, we just replace the "one > true" results object with a dummy object, which is then discarded. > > Maybe you can add a comment here to make it clearer. Oh, I understand the reason for this now. I think the comment under the if condition is fine. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:257: break On 2013/07/10 23:15:41, Dave Tu wrote: > To do this more concisely: > > pageset_start_time = time.time() > while time.time() - pageset_start_time < options.pageset_repeat_seconds: > # Stuff. Way nicer, thank you! It originally did a Do..While for when --page-repeat=0s we would still run at least one iteration. Should that be put back? https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_test.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_test.py:116: browser is torn down.""" On 2013/07/10 20:11:20, dennis_jeffrey wrote: > The first line should be a 1-line summary of the docstring. If you can't fit > everything on 1 line, add a blank line, followed by the rest: > > """First line goes here. > > Anything else goes in paragraphs down here. > """ Done.
Looking pretty good! I only have smaller comments. https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/10001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:177: # not used On 2013/07/11 01:18:55, edmundyan wrote: > On 2013/07/10 23:15:41, Dave Tu wrote: > > On 2013/07/10 21:13:09, edmundyan wrote: > > > On 2013/07/10 20:11:20, dennis_jeffrey wrote: > > > > Are you sure it's unused? I thought this was an important object that > > stores > > > > results of a run? > > > I mean the re-assigning to the variable 'results_for_current_run' is > > > unnecessary. We should just modify 'results' > > > > Yeah, this is kind of hacky and the logic of it isn't obvious. (I take no > > responsibility for it :) The test expects a results object and adds > measurement > > numbers to it. Some tests only want to record numbers for runs after the > first > > (e.g. to test only with a warm cache). To do that, we just replace the "one > > true" results object with a dummy object, which is then discarded. > > > > Maybe you can add a comment here to make it clearer. > > Oh, I understand the reason for this now. I think the comment under the if > condition is fine. If you got confused, other people will get confused too. I don't think the comment is sufficient. https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:56: self.pageset_repeat_iters = 1 I'd prefer if they were all None https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:235: if match.group(1): match.group(2) https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:239: delattr(self, "page_repeat") Single quotes. https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:241: sys.stderr.write('Usage: --page-repeat only accepts an int ' + No + https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:170: possible_browser, results, state): Indent. https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:233: for _ in xrange(int(options.page_repeat_iters))]: Can you convert this to separate for loops? It was dubious before your change, and still confusing now. Also don't need the int conversion anymore. https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_test.py (right): https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_test.py:117: browser is torn down.""" Sorry, this isn't quite what Dennis meant. The first line should be a complete one-line summary. A longer description or any clarifying statements can follow after the blank line. You need to break this sentence up into two.
https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:56: self.pageset_repeat_iters = 1 On 2013/07/11 17:28:38, Dave Tu wrote: > I'd prefer if they were all None Done, but I can't think of a clean way for _iters. Ended up setting the values to = 1 in the parser below. This is because the default value of the options are "1", so if you only enter '--page-repeat=2s' it will look like someone typed '--pageset-repeat=1', tripping the interchanging options error. https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:235: if match.group(1): On 2013/07/11 17:28:38, Dave Tu wrote: > match.group(2) Done. https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:239: delattr(self, "page_repeat") On 2013/07/11 17:28:38, Dave Tu wrote: > Single quotes. Done. https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:241: sys.stderr.write('Usage: --page-repeat only accepts an int ' + On 2013/07/11 17:28:38, Dave Tu wrote: > No + Done. https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:233: for _ in xrange(int(options.page_repeat_iters))]: On 2013/07/11 17:28:38, Dave Tu wrote: > Can you convert this to separate for loops? It was dubious before your change, > and still confusing now. Also don't need the int conversion anymore. Done. https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_test.py (right): https://codereview.chromium.org/18261009/diff/25001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_test.py:117: browser is torn down.""" On 2013/07/11 17:28:38, Dave Tu wrote: > Sorry, this isn't quite what Dennis meant. The first line should be a complete > one-line summary. A longer description or any clarifying statements can follow > after the blank line. You need to break this sentence up into two. Got it now, thanks. https://codereview.chromium.org/18261009/diff/30001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/30001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:241: while True: Note: Changed back to a Do...While because when we only set one param, e.g. '--page-repeat=5s', pageset_repeat_secs will be None and would not execute at all.
https://codereview.chromium.org/18261009/diff/30001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/30001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:241: while True: On 2013/07/11 18:55:01, edmundyan wrote: > Note: Changed back to a Do...While because when we only set one param, e.g. > '--page-repeat=5s', pageset_repeat_secs will be None and would not execute at > all. Hrm, in that case there's an implicit pageset_repeat_iters == 1, right? Maybe we should allow combinations of the two. Something like this? pageset_start_time = time.time() pageset_iters = 0 while True: if (options.pageset_repeat_secs and time.time() - pageset_start_time > options.pageset_repeat_secs): break if (options.pageset_repeat_iters and pageset_iters >= options.pageset_repeat_iters): break pageset_iters += 1 for page in pages: # More stuff.
Adding new patch #5 https://codereview.chromium.org/18261009/diff/30001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/30001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:241: while True: On 2013/07/11 21:46:50, Dave Tu wrote: > On 2013/07/11 18:55:01, edmundyan wrote: > > Note: Changed back to a Do...While because when we only set one param, e.g. > > '--page-repeat=5s', pageset_repeat_secs will be None and would not execute at > > all. > > Hrm, in that case there's an implicit pageset_repeat_iters == 1, right? Maybe we > should allow combinations of the two. Something like this? > > > pageset_start_time = time.time() > pageset_iters = 0 > while True: > if (options.pageset_repeat_secs and > time.time() - pageset_start_time > options.pageset_repeat_secs): > break > if (options.pageset_repeat_iters and > pageset_iters >= options.pageset_repeat_iters): > break > pageset_iters += 1 > > for page in pages: > # More stuff. Done. Need to use elif because we don't allow combinations for the same option.
lgtm with nit https://codereview.chromium.org/18261009/diff/37001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/37001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:239: if (options.pageset_repeat_iters != 1 and elif
https://codereview.chromium.org/18261009/diff/37001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/37001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:239: if (options.pageset_repeat_iters != 1 and On 2013/07/12 20:08:03, Dave Tu wrote: > elif I swear that was an elif at one point... Done.
Several nits. https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. remove the spaces before the # https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:4: import copy add a blank line before this https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:237: # set _iters to default to avoid interchange options error nit: Comments should generally be grammatically-correct sentences (capitalized at beginning and punctuated at end) https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:229: """Repeats each page or pageset for a specified time or iteration limit nit: add period at end of sentence https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:231: Uses the '--pageset-repeat' and '--page-repeat' cmd line options nit: add period at end of sentence https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:256: add 1 more blank line here https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_test.py (right): https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_test.py:117: This will occur before the browser is torn down.""" put the closing """ on the next line down.
Did Dennis' changes. Updated the CL's description and created a bug https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/12 21:36:18, dennis_jeffrey wrote: > remove the spaces before the # Done. https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:4: import copy On 2013/07/12 21:36:18, dennis_jeffrey wrote: > add a blank line before this Done. https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:237: # set _iters to default to avoid interchange options error On 2013/07/12 21:36:18, dennis_jeffrey wrote: > nit: Comments should generally be grammatically-correct sentences (capitalized > at beginning and punctuated at end) Done. Was also an outdated comment so I fixed that too https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:229: """Repeats each page or pageset for a specified time or iteration limit On 2013/07/12 21:36:18, dennis_jeffrey wrote: > nit: add period at end of sentence Done. https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:231: Uses the '--pageset-repeat' and '--page-repeat' cmd line options On 2013/07/12 21:36:18, dennis_jeffrey wrote: > nit: add period at end of sentence Done. https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:256: On 2013/07/12 21:36:18, dennis_jeffrey wrote: > add 1 more blank line here Done. https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_test.py (right): https://codereview.chromium.org/18261009/diff/44001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_test.py:117: This will occur before the browser is torn down.""" On 2013/07/12 21:36:18, dennis_jeffrey wrote: > put the closing """ on the next line down. Done.
LGTM
On 2013/07/12 22:25:15, dennis_jeffrey wrote: > LGTM Nat, could you take a look at this?
Sorry, I found a big bug in the repeat loop conditionals. It should be right finally. Also, found another bug that deals with skipping pages. It existed before this change, but looping over time makes the issues much more apparent. https://codereview.chromium.org/18261009/diff/47003/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/47003/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:384: if not test.CanRunForPage(page): Kind of a bug. If we ran --page-repeat=100, we check if the page has that test defined 100 times whereas one time is sufficient. This is exasperated when we run just --page-repeat=2s, where each "run" is milliseconds and we end up skipping the page thousands of times.
https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:232: this should be a helper function https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:248: so should this https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:241: if (options.pageset_repeat_secs and this is all funky to read. can you come up with a cleaner way to do it? seems like you could have an object which has has the state about start time, iters, and exposes a function RunPageMOre() and RunPagesetMore() that implement the logic. You construct the object in the caller of this function. As written, its way too hard to read to land
/me wonders if there's a PageSetRepeatOptions class you could make which deals with adding options to the parser, vaildating and converitng the opitions strings into real values, and can serve as in input into the PageRunnerRepeatState() object that i suggested above.
https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:235: str(test.action_name_to_run) + '\'') 'No pages have the action "%s"' % test.action_name_to_run Actually, I don't think you need this message. It's not entirely correct anyway, there are other reasons for which we could've skipped/removed pages, e.g. our options say to run only against WPR recorded sites, but all the sites lack a recording.
On 2013/07/16 21:32:04, nduca wrote: > /me wonders if there's a PageSetRepeatOptions class you could make which deals > with adding options to the parser, vaildating and converitng the opitions > strings into real values, and can serve as in input into the > PageRunnerRepeatState() object that i suggested above. Yes, this might be a good way to encapsulate all of the functionality into one class.
I was a bit unsure how to interpret your suggestions, especially for PageSetRepeatOptions. I don't think inheriting from BrowserOptions is correct as we need BrowserOptions to hold the complete set of all options. Then again, I am not confident in my own solution. Could use some guidance here, thanks. Also, the creation of Options() class and all_options.py is not necessary for this change. However, this will be beneficial in the future if we have additional options that need to abstracted out of BrowserOptions. https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/browser_options.py:232: On 2013/07/16 21:28:02, nduca wrote: > this should be a helper function Done in _ParseRepeatOption(), repeat_options.py https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/59001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:235: str(test.action_name_to_run) + '\'') On 2013/07/16 21:35:51, Dave Tu wrote: > 'No pages have the action "%s"' % test.action_name_to_run > > Actually, I don't think you need this message. It's not entirely correct anyway, > there are other reasons for which we could've skipped/removed pages, e.g. our > options say to run only against WPR recorded sites, but all the sites lack a > recording. Done.
:) https://codereview.chromium.org/18261009/diff/78001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/repeat_options.py (right): https://codereview.chromium.org/18261009/diff/78001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:24: def AddCommandLineOptions(self, parser): so you're pretty close i think. instead of subclassing options just make this a @staticmethod and then have BrowserOptions.__init__ do self.repeat_options = RepeatOptions() then browser_options.AddCommandLineOptions can just say RepeatOptions.AddCommandLineOptions(parser) Then as you did, call parse_args but pass the options object, e.g. self.repeat_options.UpdateFromParseResults(options) Then in the UpdateFromParseResults(), set up the local object as you did. And, get rid of the unparsed string from the options e.g. del options["page_repeat"] Then when you're all done, the browser_options will have a repeat options object that is all configured and ready for aewesomeness.
https://codereview.chromium.org/18261009/diff/78001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/repeat_options.py (right): https://codereview.chromium.org/18261009/diff/78001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:24: def AddCommandLineOptions(self, parser): On 2013/07/17 22:07:08, nduca wrote: > so you're pretty close i think. > > instead of subclassing options just make this a @staticmethod > > and then have BrowserOptions.__init__ do > self.repeat_options = RepeatOptions() > > then browser_options.AddCommandLineOptions can just say > RepeatOptions.AddCommandLineOptions(parser) > > Then as you did, call parse_args but pass the options object, e.g. > self.repeat_options.UpdateFromParseResults(options) > > Then in the UpdateFromParseResults(), set up the local object as you did. And, > get rid of the unparsed string from the options e.g. > del options["page_repeat"] > > Then when you're all done, the browser_options will have a repeat options object > that is all configured and ready for aewesomeness. Done? It's a bit different because the "options" variable you mention is actually a BrowserOptions object. I need to pass 'self' as the argument to UpdateFromParseResults. Also, the unparsed string was already being removed from the delattr() line.
lgtm on my part. dennis or david, can you please re-review once more given the recent churn? https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/repeat_options.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:45: sys.stderr.write('Usage: --%s only accepts an int ' you should pass in parser, and then call parser.error() https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:236: do you really need this as a standalone function now? its pretty compact, so you could put it int he original function now
https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/repeat_options.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. 2012 --> 2013 https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:8: add 1 more blank line here https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:22: help='Number of iterations or length of time to repeat each ' this line should be indented to line up underneath the start of the argument list in the previous line. Or, move the args on the previous line down to the next line: group.add_option( '--page-repeat', dest='page_repeat', default='1', help='Number of iterations or length of time to repeat each ' https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:28: help='Number of iterations or length of time to repeat the entire ' same indentation comment as line 22 above https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:20: from telemetry.page import page_runner_repeat move one line up to keep the imports in alphabetical order https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:238: options.repeat_options) should be indented only 4 spaces more than the line above? https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner_repeat.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. 2012 --> 2013 https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:6: add 1 more blank line here https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:17: '''Runs before we start repeating a page''' use double-quotes when defining function docstrings
lgtm still https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner_repeat.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:27: '''Runs after each completetion of a page iteration''' nit: completion. Below too.
11th time's *hopefully* the charm! https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/repeat_options.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/19 18:09:15, dennis_jeffrey wrote: > 2012 --> 2013 Done. https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:8: On 2013/07/19 18:09:15, dennis_jeffrey wrote: > add 1 more blank line here Done. https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:22: help='Number of iterations or length of time to repeat each ' On 2013/07/19 18:09:15, dennis_jeffrey wrote: > this line should be indented to line up underneath the start of the argument > list in the previous line. Or, move the args on the previous line down to the > next line: > > group.add_option( > '--page-repeat', dest='page_repeat', default='1', > help='Number of iterations or length of time to repeat each ' Done. https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/repeat_options.py:45: sys.stderr.write('Usage: --%s only accepts an int ' On 2013/07/18 21:50:10, nduca wrote: > you should pass in parser, and then call parser.error() Done. https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:20: from telemetry.page import page_runner_repeat On 2013/07/19 18:09:15, dennis_jeffrey wrote: > move one line up to keep the imports in alphabetical order Done. https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:236: On 2013/07/18 21:50:10, nduca wrote: > do you really need this as a standalone function now? its pretty compact, so you > could put it int he original function now Done. https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:238: options.repeat_options) On 2013/07/19 18:09:15, dennis_jeffrey wrote: > should be indented only 4 spaces more than the line above? You mean 2 right? https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner_repeat.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/19 18:09:15, dennis_jeffrey wrote: > 2012 --> 2013 Done. https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:6: On 2013/07/19 18:09:15, dennis_jeffrey wrote: > add 1 more blank line here Done. https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:17: '''Runs before we start repeating a page''' On 2013/07/19 18:09:15, dennis_jeffrey wrote: > use double-quotes when defining function docstrings Done. https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner_repeat.py:27: '''Runs after each completetion of a page iteration''' On 2013/07/19 21:15:23, Dave Tu wrote: > nit: completion. Below too. Done.
LGTM. I think it's ready to go! https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/88001/tools/telemetry/telemetry... tools/telemetry/telemetry/page/page_runner.py:238: options.repeat_options) On 2013/07/19 22:02:53, edmundyan wrote: > On 2013/07/19 18:09:15, dennis_jeffrey wrote: > > should be indented only 4 spaces more than the line above? > > You mean 2 right? I meant 4 (because of the hanging indent) like this, repeat_state = page_runner_repeat.PageRunnerRepeatState( options.repeat_options) But I think it's ok as you have it now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/18261009/105017
Failed to apply patch for tools/perf/perf_tools/dom_perf.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
can't find file to patch at input line 6
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: tools/perf/perf_tools/dom_perf.py
|diff --git a/tools/perf/perf_tools/dom_perf.py
b/tools/perf/perf_tools/dom_perf.py
|index
494b037ce114421c0ab0165828d54a7919f59681..e3ce2a34ce0a0f3aeb00c97b392be27ae76d1ea6
100644
|--- a/tools/perf/perf_tools/dom_perf.py
|+++ b/tools/perf/perf_tools/dom_perf.py
--------------------------
No file to patch. Skipping patch.
1 out of 1 hunk ignored
Patch: tools/perf/perf_tools/dom_perf.py
Index: tools/perf/perf_tools/dom_perf.py
diff --git a/tools/perf/perf_tools/dom_perf.py
b/tools/perf/perf_tools/dom_perf.py
index
494b037ce114421c0ab0165828d54a7919f59681..e3ce2a34ce0a0f3aeb00c97b392be27ae76d1ea6
100644
--- a/tools/perf/perf_tools/dom_perf.py
+++ b/tools/perf/perf_tools/dom_perf.py
@@ -71,7 +71,7 @@ class DomPerf(page_measurement.PageMeasurement):
finally:
tab.EvaluateJavaScript('document.cookie = "__domperf_finished=0"')
- def DidRunPageSet(self, tab, results):
+ def DidRunTest(self, tab, results):
# Now give the geometric mean as the total for the combined runs.
scores = []
for result in results.page_results:
Rebased+Merged with tip. Also found a bug when checking if we are repeating pages/pagesets. Refactored the logic into RepeatOptions class. Could use a quick look at that. https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/repeat_options.py (right): https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/repeat_options.py:54: def IsRepeating(self): PTAL https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/page_runner.py (right): https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/page_runner.py:94: if options.repeat_options.IsRepeating(): PTAL
LGTM with 2 nits. https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_options.py:4: import optparse add a blank line above this, and put the imports in alphabetical order https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/repeat_options.py (right): https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/repeat_options.py:55: """Returns True if we will be repeating pages or pagesets""" add period at end of sentence
lgtm
*cross fingers* https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_options.py:4: import optparse On 2013/07/22 22:23:45, dennis_jeffrey wrote: > add a blank line above this, and put the imports in alphabetical order This was actually reverted back to tip because I didn't end up changing imports. But Done. https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/repeat_options.py (right): https://codereview.chromium.org/18261009/diff/117001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/repeat_options.py:55: """Returns True if we will be repeating pages or pagesets""" On 2013/07/22 22:23:45, dennis_jeffrey wrote: > add period at end of sentence Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/18261009/130001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/18261009/130001
Message was sent while issue was closed.
Change committed as 213184
Message was sent while issue was closed.
I broke the build, sorry :( Uploaded a new patch that fixes the problem, but I don't know if this is the right way to override deepcopy. PTAL thanks! P.S. This still fails ./run_tests: [ FAILED ] 1 test, listed below: [ FAILED ] InspectorMemoryTest.testGetDOMStats But I get the same failure when running from tip, so I don't think it's caused by this patch.
Message was sent while issue was closed.
On 2013/07/23 21:25:28, edmundyan wrote: > I broke the build, sorry :( Since you used the trybot, this isn't your fault. Mike, any ideas why the trybot did not run the telemetry unittests for a telemetry patch? I see telemetry_unittests: skipped here: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Message was sent while issue was closed.
On 2013/07/23 21:25:28, edmundyan wrote: > I broke the build, sorry :( > > Uploaded a new patch that fixes the problem, but I don't know if this is the > right way to override deepcopy. PTAL thanks! > > P.S. This still fails ./run_tests: > [ FAILED ] 1 test, listed below: > [ FAILED ] InspectorMemoryTest.testGetDOMStats > > But I get the same failure when running from tip, so I don't think it's caused > by this patch. This works, lgtm. It's also fine to not store the compiled regex, i don't think the performance difference is significant.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/18261009/163001
Retried try job too often on win_rel for step(s) browser_tests, chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests, content_browsertests, mini_installer_test, nacl_integration, sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/edmundyan@chromium.org/18261009/73002
Had to rebase again for Tony's profiler CL. Note, I still have 2 failures from run_tests on Linux: [ FAILED ] 2 tests, listed below: [ FAILED ] InspectorMemoryTest.testGetDOMStats [ FAILED ] TestPerfProfiler.testPerfProfiler 2 FAILED TESTS Both tests were already broken in tip and should not affected by this change. Tony, are you aware of the 2nd failure, testPerfProfiler?
On Tue, Jul 23, 2013 at 8:22 PM, <edmundyan@chromium.org> wrote: > Had to rebase again for Tony's profiler CL. > > Note, I still have 2 failures from run_tests on Linux: > [ FAILED ] 2 tests, listed below: > [ FAILED ] InspectorMemoryTest.testGetDOMStats > [ FAILED ] TestPerfProfiler.testPerfProfiler > > 2 FAILED TESTS > > Both tests were already broken in tip and should not affected by this > change. > Tony, are you aware of the 2nd failure, testPerfProfiler? I just added that test and was not aware of a failure. We run the telemetry unittests on the main waterfall on mac and windows and everything is green there. This test also passed for me locally on linux. So if you could share the details of the failure message you are seeing, it would be helpful in helping me troubleshoot. > > https://codereview.chromium.org/18261009/
edmundyan@edmundyan0:~/tree/chromium/src/tools/telemetry$ ./run_tests
TestPerfProfiler.testPerfProfiler
No adb found in $PATH, fallback to checked in binary.
[----------] 1 test
[ RUN ] TestPerfProfiler.testPerfProfiler
Traceback (most recent call last):
File
"/usr/local/google/home/edmundyan/tree/chromium/src/tools/telemetry/telemetry/core/platform/profiler/perf_profiler_unittest.py",
line 30, in testPerfProfiler
'v8::internal::HeapObject::Size': 39786862
AssertionError: {} !=
{'v8::internal::StaticMarkingVisitor::MarkMapContents': 63615201,
'v8::internal: [truncated]...
- {}
+ {'WebCore::HTMLTokenizer::nextToken': 48240439,
+ 'sk_memset32_SSE2': 45121317,
+ 'v8::internal::FlexibleBodyVisitor::Visit': 31909537,
+ 'v8::internal::HeapObject::Size': 39786862,
+ 'v8::internal::LAllocator::MeetConstraintsBetween': 42913933,
+ 'v8::internal::LiveRange::CreateAssignedOperand': 42913933,
+ 'v8::internal::RelocIterator::next': 38271931,
+ 'v8::internal::Scanner::ScanIdentifierOrKeyword': 46054550,
+ 'v8::internal::StaticMarkingVisitor::MarkMapContents': 63615201,
+ 'void v8::internal::RelocInfo::Visit': 96878864}
[ FAILED ] TestPerfProfiler.testPerfProfiler (20 ms)
[----------] 1 test (19 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TestPerfProfiler.testPerfProfiler
1 FAILED TEST
On Tue, Jul 23, 2013 at 8:26 PM, Tony Gentilcore <tonyg@chromium.org> wrote:
> On Tue, Jul 23, 2013 at 8:22 PM, <edmundyan@chromium.org> wrote:
> > Had to rebase again for Tony's profiler CL.
> >
> > Note, I still have 2 failures from run_tests on Linux:
> > [ FAILED ] 2 tests, listed below:
> > [ FAILED ] InspectorMemoryTest.testGetDOMStats
> > [ FAILED ] TestPerfProfiler.testPerfProfiler
> >
> > 2 FAILED TESTS
> >
> > Both tests were already broken in tip and should not affected by this
> > change.
> > Tony, are you aware of the 2nd failure, testPerfProfiler?
>
> I just added that test and was not aware of a failure. We run the
> telemetry unittests on the main waterfall on mac and windows and
> everything is green there. This test also passed for me locally on
> linux. So if you could share the details of the failure message you
> are seeing, it would be helpful in helping me troubleshoot.
>
> >
> > https://codereview.chromium.org/18261009/
>
The commit queue silently failed to commit my binary data. Fix in http://src.chromium.org/viewvc/chrome?revision=213329&view=revision On Tue, Jul 23, 2013 at 8:29 PM, Edmund Yan <edmundyan@google.com> wrote: > edmundyan@edmundyan0:~/tree/chromium/src/tools/telemetry$ ./run_tests > TestPerfProfiler.testPerfProfiler > No adb found in $PATH, fallback to checked in binary. > [----------] 1 test > [ RUN ] TestPerfProfiler.testPerfProfiler > Traceback (most recent call last): > File > "/usr/local/google/home/edmundyan/tree/chromium/src/tools/telemetry/telemetry/core/platform/profiler/perf_profiler_unittest.py", > line 30, in testPerfProfiler > 'v8::internal::HeapObject::Size': 39786862 > AssertionError: {} != > {'v8::internal::StaticMarkingVisitor::MarkMapContents': 63615201, > 'v8::internal: [truncated]... > - {} > + {'WebCore::HTMLTokenizer::nextToken': 48240439, > + 'sk_memset32_SSE2': 45121317, > + 'v8::internal::FlexibleBodyVisitor::Visit': 31909537, > + 'v8::internal::HeapObject::Size': 39786862, > + 'v8::internal::LAllocator::MeetConstraintsBetween': 42913933, > + 'v8::internal::LiveRange::CreateAssignedOperand': 42913933, > + 'v8::internal::RelocIterator::next': 38271931, > + 'v8::internal::Scanner::ScanIdentifierOrKeyword': 46054550, > + 'v8::internal::StaticMarkingVisitor::MarkMapContents': 63615201, > + 'void v8::internal::RelocInfo::Visit': 96878864} > > [ FAILED ] TestPerfProfiler.testPerfProfiler (20 ms) > [----------] 1 test (19 ms total) > > [ PASSED ] 0 tests. > [ FAILED ] 1 test, listed below: > [ FAILED ] TestPerfProfiler.testPerfProfiler > > 1 FAILED TEST > > > > On Tue, Jul 23, 2013 at 8:26 PM, Tony Gentilcore <tonyg@chromium.org> wrote: >> >> On Tue, Jul 23, 2013 at 8:22 PM, <edmundyan@chromium.org> wrote: >> > Had to rebase again for Tony's profiler CL. >> > >> > Note, I still have 2 failures from run_tests on Linux: >> > [ FAILED ] 2 tests, listed below: >> > [ FAILED ] InspectorMemoryTest.testGetDOMStats >> > [ FAILED ] TestPerfProfiler.testPerfProfiler >> > >> > 2 FAILED TESTS >> > >> > Both tests were already broken in tip and should not affected by this >> > change. >> > Tony, are you aware of the 2nd failure, testPerfProfiler? >> >> I just added that test and was not aware of a failure. We run the >> telemetry unittests on the main waterfall on mac and windows and >> everything is green there. This test also passed for me locally on >> linux. So if you could share the details of the failure message you >> are seeing, it would be helpful in helping me troubleshoot. >> >> > >> > https://codereview.chromium.org/18261009/ > >
Message was sent while issue was closed.
Change committed as 213360
Message was sent while issue was closed.
I'm getting this error this morning: $ tools/perf/run_measurement --browser=system loading_profile http://www.google.com/ Traceback (most recent call last): File "tools/perf/run_measurement", line 97, in <module> sys.exit(main()) File "tools/perf/run_measurement", line 94, in main sys.exit(runner.Run(os.path.dirname(__file__), page_set_filenames)) File "/usr/local/google/code/chromium/src/tools/telemetry/telemetry/page/page_test_runner.py", line 40, in Run page_set_filenames) File "/usr/local/google/code/chromium/src/tools/telemetry/telemetry/page/page_test_runner.py", line 150, in ParseCommandLine _, self._args = self._parser.parse_args() File "/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/browser_options.py", line 204, in ParseArgs self.repeat_options.UpdateFromParseResults(self, parser) File "/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/repeat_options.py", line 55, in UpdateFromParseResults self._ParseRepeatOption(browser_options, 'page_repeat', parser) File "/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/repeat_options.py", line 40, in _ParseRepeatOption input_str, '')) File "/usr/lib/python2.7/re.py", line 137, in match return _compile(pattern, flags).match(string) TypeError: expected string or buffer Is there a simple fix or should we revert and try again?
Message was sent while issue was closed.
I know what the problem is, and it's an easy fix (uploaded partial patch). However, I'm having technical problems running page_cycler on my machine so I'm a bit stalled. I'm okay with reverting and trying again.
Message was sent while issue was closed.
On 2013/07/24 17:51:49, edmundyan wrote: > I know what the problem is, and it's an easy fix (uploaded partial patch). > However, I'm having technical problems running page_cycler on my machine so I'm > a bit stalled. I'm okay with reverting and trying again. Maybe this error repros without running the page cyclers? My repro was with the loading_profile measurement.
Message was sent while issue was closed.
This change has majorly broken the Blink perf bots, with traces like:
Traceback (most recent call last):
File "src/tools/perf/run_measurement", line 97, in <module>
sys.exit(main())
File "src/tools/perf/run_measurement", line 94, in main
sys.exit(runner.Run(os.path.dirname(__file__), page_set_filenames))
File
"/b/build/slave/Linux_Perf/build/src/tools/telemetry/telemetry/page/page_test_runner.py",
line 40, in Run
page_set_filenames)
File
"/b/build/slave/Linux_Perf/build/src/tools/telemetry/telemetry/page/page_test_runner.py",
line 150, in ParseCommandLine
_, self._args = self._parser.parse_args()
File
"/b/build/slave/Linux_Perf/build/src/tools/telemetry/telemetry/core/browser_options.py",
line 204, in ParseArgs
self.repeat_options.UpdateFromParseResults(self, parser)
File
"/b/build/slave/Linux_Perf/build/src/tools/telemetry/telemetry/core/repeat_options.py",
line 56, in UpdateFromParseResults
self._ParseRepeatOption(browser_options, 'pageset_repeat', parser)
File
"/b/build/slave/Linux_Perf/build/src/tools/telemetry/telemetry/core/repeat_options.py",
line 40, in _ParseRepeatOption
input_str, ''))
File "/usr/lib/python2.7/re.py", line 137, in match
return _compile(pattern, flags).match(string)
Please fix ASAP
Message was sent while issue was closed.
adamk, sorry for the delay. I kicked this off on commit queue because dcommit was not working for me. I am following up why there seems to be errors on win_rel and linux_aura https://codereview.chromium.org/19700009/ |
