Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12092033:
First pass on tool to bisect across range of revisions to help narrow down where a regression in a … (Closed)
|
Created:
7 years, 10 months ago by shatch Modified:
7 years, 10 months ago CC:
chromium-reviews, sullivan, nduca, ojan, szager1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFirst pass on tool to bisect across range of revisions to help narrow down where a regression in a performance metric occurred. The tool will also attempt to track down regressions caused by WebKit, Skia, and V8 by bisecting those depots as well. At the moment, the tool only works on linux based machines using the git workflow.
An example usage:
./tools/bisect-perf-regression.py -c\
"out/Release/performance_ui_tests --gtest_filter=ShutdownTest.SimpleUserQuit"\
-g 1f6e67861535121c5c819c16a666f2436c207e7b\
-b b732f23b4f81c382db0b23b9035f3dadc7d925bb\
-m shutdown/simple-user-quit
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180082
Patch Set 1 #
Total comments: 82
Patch Set 2 : Cleanup, refactoring, style fixes from review. #
Total comments: 17
Patch Set 3 : Style fixes and additional comments from review. #Patch Set 4 : Removed skia. #Patch Set 5 : Messed some git stuff up. #Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:25: DEPOT_PATHS_FROM_SRC = { DEPOT_WEBKIT : '/third_party/WebKit', Are these the right depots? https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:172: class SourceControl(object): Will we ever extend this to the svn workflow? If not, I probably didn't need to do this. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:221: results = self.RunGit(['checkout', revision]) At the moment, I update the third party libs (WebKit, Skia, V8) by going into their directories and calling "git checkout <revision>", but I'm not clear if this will cause any issues. Is there a way of doing this through gclient? https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:247: def IsInProperBranch(self): I found that gclient sync --revision failed after repeated syncs if I wasn't in the master branch. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:328: if gyp_var != None and 'ninja' in gyp_var: Wasn't sure if I needed to do a check for this, but my build failed initially since I had ninja set up on my laptop. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:696: deps_file = imp.load_source('gclient', '../.gclient') Is this an accurate way of checking their source control?
I bet this is for tonyg.
Overall looks really solid. Most of my nits are just stylistic. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:14: range. An example usage like you have in the CL description would be great here. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:20: DEPOT_NAMES = [DEPOT_WEBKIT, DEPOT_SKIA, DEPOT_V8] I'd recommend killing this variable and just inlining DEPOT_DEPS_NAME.keys() https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:25: DEPOT_PATHS_FROM_SRC = { DEPOT_WEBKIT : '/third_party/WebKit', On 2013/01/29 02:22:09, shatch wrote: > Are these the right depots? yep https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:33: ERROR_MESSAGE_OS_NOT_SUPPORTED = "Sorry, this platform isn't supported yet." I have a weak preference to just inline all these messages. I think it would make the code a little bit easier to follow. Especially for the ones with formatting strings in them. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:82: import subprocess Please put the imports above the constants https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:86: GLOBAL_BISECT_OPTIONS = {'build' : False, This global is begging to be refactored as a class instead. Is there a clean way to work that out? https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:135: """Checks whether or not the given string can be converted to a floating Check out the style guide's section about function level comments: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:144: return result Here and below, I'd eliminate the result variable and just inline the return statements. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:162: shell = (os.name == 'nt') I've always seen sys.platform == 'win32'. Is this equivalent? https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:172: class SourceControl(object): On 2013/01/29 02:22:09, shatch wrote: > Will we ever extend this to the svn workflow? If not, I probably didn't need to > do this. Probably not, but it is fine to have this abstraction. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:199: good revision.""" This comment assumes it is between the bad and good revision, but it is really just between |revision_range_start| and |revision_range_end| https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:201: (log_output, return_code) = self.RunGit(['log', Should we add an: assert not return_code Comment applies to other places where return_code is unused as well. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:221: results = self.RunGit(['checkout', revision]) On 2013/01/29 02:22:09, shatch wrote: > At the moment, I update the third party libs (WebKit, Skia, V8) by going into > their directories and calling "git checkout <revision>", but I'm not clear if > this will cause any issues. Is there a way of doing this through gclient? No, that seems right. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:225: def ResolveToRevision(self, revision_to_check): Recommend naming this ResolveSvnToGitRevision() https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:228: if not(IsStringInt(revision_to_check)): if not IsStringInt(revision_to_check): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:247: def IsInProperBranch(self): On 2013/01/29 02:22:09, shatch wrote: > I found that gclient sync --revision failed after repeated syncs if I wasn't in > the master branch. Why not name this IsInMasterBranch() https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:273: self.depot_cwd[d] = self.src_cwd + DEPOT_PATHS_FROM_SRC[d] You could eliminate the DEPOT_PATHS_FROM_SRC constant and just inline: DEPOT_DEPS_NAME[d][3:] https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:299: rxp = re.compile(rxp) Just inline the re.compile() call on the line above? Also, would be nice to have a descriptive variable name. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:315: """Builds chrome and the performance_uit_tests suite on the current s/uit/ui/ https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:328: if gyp_var != None and 'ninja' in gyp_var: On 2013/01/29 02:22:09, shatch wrote: > Wasn't sure if I needed to do a check for this, but my build failed initially > since I had ninja set up on my laptop. looks good https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:337: 'BUILDTYPE=Release', Doesn't this have to go before the make command? Did you test the non-ninja path? https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:349: #print('build exited with: %d' % (return_code)) Remove this, or perhaps better yet import logging and make this a logging.debug() line. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:360: return results[1] == 0 return not results[1] http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#True/False_eva... https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:383: "(\s)*\[(\s)*(?P<values>[0-9,.]+)\]" These aren't always a list of values in []. Sometimes it is just a bare number. See GraphingLogProcessor here: https://code.google.com/searchframe#OAMlx_jo-ck/tools/build/scripts/slave/pro... https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:413: if len(metric_values) > 0: if metric_values: https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:459: def Run(self, command_to_run, bad_revision_in, good_revision_in, metric): This method is a bit unruly, is there a ways to factor out some helpers? Perhaps the comments are a good indicator of logical parts to break out. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:464: @param command_to_run Specify the command to execute the performance test. See http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:478: results['error'] = 'Could\'t resolve [%s] to SHA1.'%(bad_revision_in,) Space around % here and below. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:530: if bad_revision_run_results[1] != 0: if bad_revision_run_results[1]: here and below. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:536: command_to_run, indentation https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:636: break This part is really hard to read. I'm hoping factoring out some helper methods will help, but if not, maybe some brief comments here would. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:652: assert False assert False, 'some description' https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:696: deps_file = imp.load_source('gclient', '../.gclient') On 2013/01/29 02:22:09, shatch wrote: > Is this an accurate way of checking their source control? Not sure. Maybe check the message returned by "svn status" or "git status"? https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:722: type = 'str', Google python style is to omit spaces around the "=" when passing args by keyword. See: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Whites... https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:751: if opts.command is None: if not opts.command: https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:798: if not(source_control.IsInProperBranch()): if not source_control.IsInProperBranch(): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:822: f = v['value'] Recommend some more descriptive variable names here.
I didn't look at the whole file, but noticed this near the top. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:25: DEPOT_PATHS_FROM_SRC = { DEPOT_WEBKIT : '/third_party/WebKit', On 2013/01/29 18:49:33, tonyg wrote: > On 2013/01/29 02:22:09, shatch wrote: > > Are these the right depots? > > yep Skia uses third_party/skia/{src,include,gyp}. Don't we need to sync all of these together?
New snapshot uploaded. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:14: range. On 2013/01/29 18:49:33, tonyg wrote: > An example usage like you have in the CL description would be great here. Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:20: DEPOT_NAMES = [DEPOT_WEBKIT, DEPOT_SKIA, DEPOT_V8] On 2013/01/29 18:49:33, tonyg wrote: > I'd recommend killing this variable and just inlining DEPOT_DEPS_NAME.keys() Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:25: DEPOT_PATHS_FROM_SRC = { DEPOT_WEBKIT : '/third_party/WebKit', On 2013/01/29 20:10:36, tony wrote: > On 2013/01/29 18:49:33, tonyg wrote: > > On 2013/01/29 02:22:09, shatch wrote: > > > Are these the right depots? > > > > yep > > Skia uses third_party/skia/{src,include,gyp}. Don't we need to sync all of these > together? Are these all the same depot? They seem to be 3 different .git files. I'm a bit unclear as to how to sync them together. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:33: ERROR_MESSAGE_OS_NOT_SUPPORTED = "Sorry, this platform isn't supported yet." On 2013/01/29 18:49:33, tonyg wrote: > I have a weak preference to just inline all these messages. I think it would > make the code a little bit easier to follow. Especially for the ones with > formatting strings in them. Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:82: import subprocess On 2013/01/29 18:49:33, tonyg wrote: > Please put the imports above the constants Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:86: GLOBAL_BISECT_OPTIONS = {'build' : False, On 2013/01/29 18:49:33, tonyg wrote: > This global is begging to be refactored as a class instead. Is there a clean way > to work that out? Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:135: """Checks whether or not the given string can be converted to a floating On 2013/01/29 18:49:33, tonyg wrote: > Check out the style guide's section about function level comments: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:144: return result On 2013/01/29 18:49:33, tonyg wrote: > Here and below, I'd eliminate the result variable and just inline the return > statements. Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:162: shell = (os.name == 'nt') On 2013/01/29 18:49:33, tonyg wrote: > I've always seen sys.platform == 'win32'. Is this equivalent? Honestly I just grabbed this function from another py file, there were variants of it in a few different places. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:199: good revision.""" On 2013/01/29 18:49:33, tonyg wrote: > This comment assumes it is between the bad and good revision, but it is really > just between |revision_range_start| and |revision_range_end| Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:201: (log_output, return_code) = self.RunGit(['log', On 2013/01/29 18:49:33, tonyg wrote: > Should we add an: > assert not return_code > > Comment applies to other places where return_code is unused as well. Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:225: def ResolveToRevision(self, revision_to_check): On 2013/01/29 18:49:33, tonyg wrote: > Recommend naming this ResolveSvnToGitRevision() Wasn't sure if I should refer to specific workflows since there is the abstraction over top. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:228: if not(IsStringInt(revision_to_check)): On 2013/01/29 18:49:33, tonyg wrote: > if not IsStringInt(revision_to_check): Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:247: def IsInProperBranch(self): On 2013/01/29 18:49:33, tonyg wrote: > On 2013/01/29 02:22:09, shatch wrote: > > I found that gclient sync --revision failed after repeated syncs if I wasn't > in > > the master branch. > > Why not name this IsInMasterBranch() My thought was as part of the abstraction, I shouldn't refer to specific workflows. If I were to add svn later, I wasn't sure if "master" branch was relevant there. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:273: self.depot_cwd[d] = self.src_cwd + DEPOT_PATHS_FROM_SRC[d] On 2013/01/29 18:49:33, tonyg wrote: > You could eliminate the DEPOT_PATHS_FROM_SRC constant and just inline: > DEPOT_DEPS_NAME[d][3:] Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:299: rxp = re.compile(rxp) On 2013/01/29 18:49:33, tonyg wrote: > Just inline the re.compile() call on the line above? > > Also, would be nice to have a descriptive variable name. Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:315: """Builds chrome and the performance_uit_tests suite on the current On 2013/01/29 18:49:33, tonyg wrote: > s/uit/ui/ Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:337: 'BUILDTYPE=Release', On 2013/01/29 18:49:33, tonyg wrote: > Doesn't this have to go before the make command? Did you test the non-ninja > path? It seemed legit, got it from the linux build instructions page: https://code.google.com/p/chromium/wiki/LinuxBuildInstructions#Release_mode Used the ninja path on my laptop and make on my desktop. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:349: #print('build exited with: %d' % (return_code)) On 2013/01/29 18:49:33, tonyg wrote: > Remove this, or perhaps better yet import logging and make this a > logging.debug() line. Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:360: return results[1] == 0 On 2013/01/29 18:49:33, tonyg wrote: > return not results[1] > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#True/False_eva... Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:383: "(\s)*\[(\s)*(?P<values>[0-9,.]+)\]" On 2013/01/29 18:49:33, tonyg wrote: > These aren't always a list of values in []. Sometimes it is just a bare number. > See GraphingLogProcessor here: > https://code.google.com/searchframe#OAMlx_jo-ck/tools/build/scripts/slave/pro... Won't the first search cover that case? https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:413: if len(metric_values) > 0: On 2013/01/29 18:49:33, tonyg wrote: > if metric_values: Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:464: @param command_to_run Specify the command to execute the performance test. On 2013/01/29 18:49:33, tonyg wrote: > See http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:478: results['error'] = 'Could\'t resolve [%s] to SHA1.'%(bad_revision_in,) On 2013/01/29 18:49:33, tonyg wrote: > Space around % here and below. Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:530: if bad_revision_run_results[1] != 0: On 2013/01/29 18:49:33, tonyg wrote: > if bad_revision_run_results[1]: > here and below. Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:536: command_to_run, On 2013/01/29 18:49:33, tonyg wrote: > indentation Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:652: assert False On 2013/01/29 18:49:33, tonyg wrote: > assert False, 'some description' Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:696: deps_file = imp.load_source('gclient', '../.gclient') On 2013/01/29 18:49:33, tonyg wrote: > On 2013/01/29 02:22:09, shatch wrote: > > Is this an accurate way of checking their source control? > > Not sure. Maybe check the message returned by "svn status" or "git status"? Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:722: type = 'str', On 2013/01/29 18:49:33, tonyg wrote: > Google python style is to omit spaces around the "=" when passing args by > keyword. See: > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Whites... Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:751: if opts.command is None: On 2013/01/29 18:49:33, tonyg wrote: > if not opts.command: Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:798: if not(source_control.IsInProperBranch()): On 2013/01/29 18:49:33, tonyg wrote: > if not source_control.IsInProperBranch(): Done. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:822: f = v['value'] On 2013/01/29 18:49:33, tonyg wrote: > Recommend some more descriptive variable names here. Done.
https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:25: DEPOT_PATHS_FROM_SRC = { DEPOT_WEBKIT : '/third_party/WebKit', On 2013/01/30 03:23:02, shatch wrote: > On 2013/01/29 20:10:36, tony wrote: > > On 2013/01/29 18:49:33, tonyg wrote: > > > On 2013/01/29 02:22:09, shatch wrote: > > > > Are these the right depots? > > > > > > yep > > > > Skia uses third_party/skia/{src,include,gyp}. Don't we need to sync all of > these > > together? > > Are these all the same depot? They seem to be 3 different .git files. I'm a bit > unclear as to how to sync them together. They're all from the same skia svn repo. I'm not sure what the correct way to sync the git repos are. Maybe szager knows how the git hashes are produced from the svn revision.
Just some minor nits. Everything else looks good to me. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:162: shell = (os.name == 'nt') On 2013/01/30 03:23:02, shatch wrote: > On 2013/01/29 18:49:33, tonyg wrote: > > I've always seen sys.platform == 'win32'. Is this equivalent? > > Honestly I just grabbed this function from another py file, there were variants > of it in a few different places. It is fine to leave as-is then. This script will probably need some work the first time it is used on windows anyway. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:337: 'BUILDTYPE=Release', On 2013/01/30 03:23:02, shatch wrote: > On 2013/01/29 18:49:33, tonyg wrote: > > Doesn't this have to go before the make command? Did you test the non-ninja > > path? > > It seemed legit, got it from the linux build instructions page: > https://code.google.com/p/chromium/wiki/LinuxBuildInstructions#Release_mode > > Used the ninja path on my laptop and make on my desktop. OK, sorry I was unfamiliar with that syntax https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:383: "(\s)*\[(\s)*(?P<values>[0-9,.]+)\]" On 2013/01/30 03:23:02, shatch wrote: > On 2013/01/29 18:49:33, tonyg wrote: > > These aren't always a list of values in []. Sometimes it is just a bare > number. > > See GraphingLogProcessor here: > > > https://code.google.com/searchframe#OAMlx_jo-ck/tools/build/scripts/slave/pro... > > Won't the first search cover that case? Oh, I follow now. Thanks. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:22: -b b732f23b4f81c382db0b23b9035f3dadc7d925bb\ Recommend putting svn revisions in the example since that will be the common use case. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:45: ############################################################################### Now that most of the comments are gone, this bar isn't really as necessary https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:167: ' --first-parent %s' % (svn_pattern,) To avoid repeating the command here, I recommend extracting out a cmd list and using it here. cmd = ['log', ...] self.RunGit(cmd) 'An error occurred while running %s' % ' '.join(cmd) ditto in other assertions. It also makes me wonder whether the assertion should just be moved up into RunGit(). https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:194: """If the user supplied an SVN revision, try to resolve it to a Probably shouldn't mention the user in this function level comment https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:261: self.depot_cwd[d] = self.src_cwd + DEPOT_DEPS_NAME[d][3:] Perhaps a brief comment about the [3:]. Maybe # skip 'src' https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:290: rxp = re.compile(".git@(?P<revision>[a-zA-Z0-9]+)") Is it worth being more specific and using a-fA-F instead of a-zA-Z https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:889: (output, return_code) = RunProcess(['git', Maybe RunGit should become an @staticmethod so that it can be used here. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:914: 'Must be later than good revision. ') Might be worth stating in the help strings that these are git or svn revisions.
New snapshot uploaded. https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:25: DEPOT_PATHS_FROM_SRC = { DEPOT_WEBKIT : '/third_party/WebKit', Will follow up with him. On 2013/01/30 18:13:29, tony wrote: > On 2013/01/30 03:23:02, shatch wrote: > > On 2013/01/29 20:10:36, tony wrote: > > > On 2013/01/29 18:49:33, tonyg wrote: > > > > On 2013/01/29 02:22:09, shatch wrote: > > > > > Are these the right depots? > > > > > > > > yep > > > > > > Skia uses third_party/skia/{src,include,gyp}. Don't we need to sync all of > > these > > > together? > > > > Are these all the same depot? They seem to be 3 different .git files. I'm a > bit > > unclear as to how to sync them together. > > They're all from the same skia svn repo. I'm not sure what the correct way to > sync the git repos are. Maybe szager knows how the git hashes are produced from > the svn revision. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:22: -b b732f23b4f81c382db0b23b9035f3dadc7d925bb\ On 2013/01/30 18:26:28, tonyg wrote: > Recommend putting svn revisions in the example since that will be the common use > case. Done. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:45: ############################################################################### On 2013/01/30 18:26:28, tonyg wrote: > Now that most of the comments are gone, this bar isn't really as necessary Done. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:167: ' --first-parent %s' % (svn_pattern,) On 2013/01/30 18:26:28, tonyg wrote: > To avoid repeating the command here, I recommend extracting out a cmd list and > using it here. > cmd = ['log', ...] > self.RunGit(cmd) > 'An error occurred while running %s' % ' '.join(cmd) > > ditto in other assertions. > > It also makes me wonder whether the assertion should just be moved up into > RunGit(). Done. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:194: """If the user supplied an SVN revision, try to resolve it to a On 2013/01/30 18:26:28, tonyg wrote: > Probably shouldn't mention the user in this function level comment Done. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:261: self.depot_cwd[d] = self.src_cwd + DEPOT_DEPS_NAME[d][3:] On 2013/01/30 18:26:28, tonyg wrote: > Perhaps a brief comment about the [3:]. Maybe # skip 'src' Done. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:290: rxp = re.compile(".git@(?P<revision>[a-zA-Z0-9]+)") On 2013/01/30 18:26:28, tonyg wrote: > Is it worth being more specific and using a-fA-F instead of a-zA-Z Done. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:881: def ParseGClientForSourceControl(): This name doesn't reflect what the function does anymore, will change it. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:889: (output, return_code) = RunProcess(['git', On 2013/01/30 18:26:28, tonyg wrote: > Maybe RunGit should become an @staticmethod so that it can be used here. Done. https://codereview.chromium.org/12092033/diff/6002/tools/bisect-perf-regressi... tools/bisect-perf-regression.py:914: 'Must be later than good revision. ') On 2013/01/30 18:26:28, tonyg wrote: > Might be worth stating in the help strings that these are git or svn revisions. Done.
lgtm
https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.py File tools/bisect-perf-regression.py (right): https://codereview.chromium.org/12092033/diff/1/tools/bisect-perf-regression.... tools/bisect-perf-regression.py:25: DEPOT_PATHS_FROM_SRC = { DEPOT_WEBKIT : '/third_party/WebKit', On 2013/01/30 19:50:29, shatch wrote: > Will follow up with him. > > On 2013/01/30 18:13:29, tony wrote: > > On 2013/01/30 03:23:02, shatch wrote: > > > On 2013/01/29 20:10:36, tony wrote: > > > > On 2013/01/29 18:49:33, tonyg wrote: > > > > > On 2013/01/29 02:22:09, shatch wrote: > > > > > > Are these the right depots? > > > > > > > > > > yep > > > > > > > > Skia uses third_party/skia/{src,include,gyp}. Don't we need to sync all of > > > these > > > > together? > > > > > > Are these all the same depot? They seem to be 3 different .git files. I'm a > > bit > > > unclear as to how to sync them together. > > > > They're all from the same skia svn repo. I'm not sure what the correct way to > > sync the git repos are. Maybe szager knows how the git hashes are produced > from > > the svn revision. > I think you need to treat skia/include, skia/gyp, and skia/src separately. In both DEPS and .DEPS.git, they are treated as three separate checkouts, even though they all come from the same svn repo.
On 2013/01/31 01:51:27, szager1 wrote: > I think you need to treat skia/include, skia/gyp, and skia/src separately. In > both DEPS and .DEPS.git, they are treated as three separate checkouts, even > though they all come from the same svn repo. Right, my question is if you sync skia/include to svn revision X, you need to also sync skia/gyp and skia/src to revision X. With svn, that's easy, but how would you do this with a git checkout?
New snapshot uploaded. Going with Tony's suggestion, Skia bisecting is removed and can be added as a follow up patch.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonhatch@chromium.org/12092033/14001
Failed to apply patch for tools/bisect-perf-regression.py:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file tools/bisect-perf-regression.py
Hunk #1 FAILED at 9.
Hunk #2 FAILED at 82.
Hunk #3 FAILED at 169.
Hunk #4 FAILED at 177.
Hunk #5 FAILED at 188.
Hunk #6 FAILED at 210.
Hunk #7 FAILED at 262.
Hunk #8 FAILED at 282.
Hunk #9 FAILED at 295.
Hunk #10 FAILED at 312.
Hunk #11 FAILED at 346.
Hunk #12 FAILED at 392.
Hunk #13 FAILED at 403.
Hunk #14 FAILED at 410.
Hunk #15 FAILED at 438.
Hunk #16 FAILED at 475.
Hunk #17 FAILED at 511.
Hunk #18 FAILED at 568.
Hunk #19 FAILED at 642.
Hunk #20 FAILED at 684.
Hunk #21 FAILED at 719.
21 out of 21 hunks FAILED -- saving rejects to file
tools/bisect-perf-regression.py.rej
Patch: tools/bisect-perf-regression.py
Index: tools/bisect-perf-regression.py
diff --git a/tools/bisect-perf-regression.py b/tools/bisect-perf-regression.py
index
8f126df091df9b1eebd311614fc9a98d74be5068..64c07db3e2d7f46f46019d15bd569f56f9c28471
100755
--- a/tools/bisect-perf-regression.py
+++ b/tools/bisect-perf-regression.py
@@ -9,69 +9,32 @@ This script bisects a series of changelists using binary
search. It starts at
a bad revision where a performance metric has regressed, and asks for a last
known-good revision. It will then binary search across this revision range by
syncing, building, and running a performance test. If the change is
-suspected to occur as a result of WebKit/Skia/V8 changes, the script will
+suspected to occur as a result of WebKit/V8 changes, the script will
further bisect changes to those depots and attempt to narrow down the revision
range.
-"""
-
-DEPOT_WEBKIT = 'webkit'
-DEPOT_SKIA = 'skia'
-DEPOT_V8 = 'v8'
-DEPOT_NAMES = [DEPOT_WEBKIT, DEPOT_SKIA, DEPOT_V8]
-DEPOT_DEPS_NAME = { DEPOT_WEBKIT : "src/third_party/WebKit",
- DEPOT_SKIA : 'src/third_party/skia/src',
- DEPOT_V8 : 'src/v8' }
-
-DEPOT_PATHS_FROM_SRC = { DEPOT_WEBKIT : '/third_party/WebKit',
- DEPOT_SKIA : '/third_party/skia/src',
- DEPOT_V8 : '/v8' }
-
-FILE_DEPS_GIT = '.DEPS.git'
-
-SUPPORTED_OS_TYPES = ['posix']
-
-ERROR_MESSAGE_OS_NOT_SUPPORTED = "Sorry, this platform isn't supported yet."
-ERROR_MESSAGE_SVN_UNSUPPORTED = "Sorry, only the git workflow is supported"\
- " at the moment."
-ERROR_MESSAGE_GCLIENT_NOT_FOUND = "An error occurred trying to read .gclient"\
- " file. Check that you are running the tool"\
- " from $chromium/src"
+An example usage (using svn cl's):
-ERROR_MESSAGE_IMPORT_GCLIENT = "An error occurred while importing .gclient"\
- " file."
+./tools/bisect-perf-regression.py -c\
+"out/Release/performance_ui_tests --gtest_filter=ShutdownTest.SimpleUserQuit"\
+-g 168222 -b 168232 -m shutdown/simple-user-quit
-ERROR_MISSING_PARAMETER = 'Error: missing required parameter:'
-ERROR_MISSING_COMMAND = ERROR_MISSING_PARAMETER + ' --command'
-ERROR_MISSING_GOOD_REVISION = ERROR_MISSING_PARAMETER + ' --good_revision'
-ERROR_MISSING_BAD_REVISION = ERROR_MISSING_PARAMETER + ' --bad_revision'
-ERROR_MISSING_METRIC = ERROR_MISSING_PARAMETER + ' --metric'
+Be aware that if you're using the git workflow and specify an svn revision,
+the script will attempt to find the git SHA1 where svn changes up to that
+revision were merged in.
-ERROR_RETRIEVE_REVISION_RANGE = 'An error occurred attempting to retrieve'\
- ' revision range: [%s..%s]'
-ERROR_RETRIEVE_REVISION = 'An error occurred attempting to retrieve revision:'\
- '[%s]'
-ERROR_PERF_TEST_NO_VALUES = 'No values returned from performance test.'
-ERROR_PERF_TEST_FAILED_RUN = 'Failed to run performance test.'
+An example usage (using git hashes):
-ERROR_FAILED_DEPS_PARSE = 'Failed to parse DEPS file for WebKit revision.'
-ERROR_FAILED_BUILD = 'Failed to build revision: [%s]'
-ERROR_FAILED_GCLIENT_RUNHOOKS = 'Failed to run [gclient runhooks].'
-ERROR_FAILED_SYNC = 'Failed to sync revision: [%s]'
-ERROR_INCORRECT_BRANCH = "You must switch to master branch to run bisection."
-
-MESSAGE_REGRESSION_DEPOT_METRIC = 'Regression in metric:%s appears to be the'\
- ' result of changes in [%s].'
-MESSAGE_BISECT_DEPOT = 'Revisions to bisect on [%s]:'
-MESSAGE_BISECT_SRC ='Revisions to bisect on chromium/src:'
-MESSAGE_GATHERING_REFERENCE = 'Gathering reference values for bisection.'
-MESSAGE_GATHERING_REVISIONS = 'Gathering revision range for bisection.'
-MESSAGE_WORK_ON_REVISION = 'Working on revision: [%s]'
+./tools/bisect-perf-regression.py -c\
+"out/Release/performance_ui_tests --gtest_filter=ShutdownTest.SimpleUserQuit"\
+-g 1f6e67861535121c5c819c16a666f2436c207e7b\
+-b b732f23b4f81c382db0b23b9035f3dadc7d925bb\
+-m shutdown/simple-user-quit
+"""
-###############################################################################
import re
import os
@@ -82,82 +45,58 @@ import optparse
import subprocess
+DEPOT_DEPS_NAME = { 'webkit' : "src/third_party/WebKit",
+ 'v8' : 'src/v8' }
+DEPOT_NAMES = DEPOT_DEPS_NAME.keys()
-GLOBAL_BISECT_OPTIONS = {'build' : False,
- 'sync' : False,
- 'perf' : False,
- 'goma' : False}
-
-def SetGlobalGomaFlag(flag):
- """Sets global flag for using goma with builds."""
- global GLOBAL_BISECT_OPTIONS
-
- GLOBAL_BISECT_OPTIONS['goma'] = flag
-
-
-def IsGlobalGomaFlagEnabled():
- global GLOBAL_BISECT_OPTIONS
-
- return GLOBAL_BISECT_OPTIONS['goma']
-
-
-def SetDebugIgnoreFlags(build_flag, sync_flag, perf_flag):
- """Sets global flags for ignoring builds, syncs, and perf tests."""
- global GLOBAL_BISECT_OPTIONS
-
- GLOBAL_BISECT_OPTIONS['build'] = build_flag
- GLOBAL_BISECT_OPTIONS['sync'] = sync_flag
- GLOBAL_BISECT_OPTIONS['perf'] = perf_flag
-
-
-def IsDebugIgnoreBuildEnabled():
- """Returns whether build commands are being ignored."""
- global GLOBAL_BISECT_OPTIONS
-
- return GLOBAL_BISECT_OPTIONS['build']
-
-
-def IsDebugIgnoreSyncEnabled():
- """Returns whether sync commands are being ignored."""
- global GLOBAL_BISECT_OPTIONS
-
- return GLOBAL_BISECT_OPTIONS['sync']
-
-
-def IsDebugIgnorePerfTestEnabled():
- """Returns whether performance test commands are being ignored."""
- global GLOBAL_BISECT_OPTIONS
+FILE_DEPS_GIT = '.DEPS.git'
- return GLOBAL_BISECT_OPTIONS['perf']
def IsStringFloat(string_to_check):
"""Checks whether or not the given string can be converted to a floating
- point number."""
+ point number.
+
+ Args:
+ string_to_check: Input string to check if it can be converted to a float.
+
+ Returns:
+ True if the string can be converted to a float.
+ """
try:
float(string_to_check)
- result = True
+ return True
except ValueError:
- result = False
-
- return result
+ return False
def IsStringInt(string_to_check):
- """Checks whether or not the given string can be converted to a integer."""
+ """Checks whether or not the given string can be converted to a integer.
+
+ Args:
+ string_to_check: Input string to check if it can be converted to an int.
+
+ Returns:
+ True if the string can be converted to an int.
+ """
try:
int(string_to_check)
- result = True
+ return True
except ValueError:
- result = False
-
- return result
+ return False
def RunProcess(command):
- """Run an arbitrary command, returning its output and return code."""
+ """Run an arbitrary command, returning its output and return code.
+
+ Args:
+ command: A list containing the command and args to execute.
+
+ Returns:
+ A tuple of the output and return code.
+ """
# On Windows, use shell=True to get PATH interpretation.
shell = (os.name == 'nt')
proc = subprocess.Popen(command,
@@ -169,6 +108,20 @@ def RunProcess(command):
return (out, proc.returncode)
+def RunGit(command):
+ """Run a git subcommand, returning its output and return code.
+
+ Args:
+ command: A list containing the args to git.
+
+ Returns:
+ A tuple of the output and return code.
+ """
+ command = ['git'] + command
+
+ return RunProcess(command)
+
+
class SourceControl(object):
"""SourceControl is an abstraction over the underlying source control
system used for chromium. For now only git is supported, but in the
@@ -177,7 +130,16 @@ class SourceControl(object):
super(SourceControl, self).__init__()
def SyncToRevisionWithGClient(self, revision):
- """Uses gclient to sync to the specified revision."""
+ """Uses gclient to sync to the specified revision.
+
+ ie. gclient sync --revision <revision>
+
+ Args:
+ revision: The git SHA1 or svn CL (depending on workflow).
+
+ Returns:
+ A tuple of the output and return code.
+ """
args = ['gclient', 'sync', '--revision', revision]
return RunProcess(args)
@@ -188,21 +150,24 @@ class GitSourceControl(SourceControl):
def __init__(self):
super(GitSourceControl, self).__init__()
- def RunGit(self, command):
- """Run a git subcommand, returning its output and return code."""
- command = ['git'] + command
+ def GetRevisionList(self, revision_range_end, revision_range_start):
+ """Retrieves a list of revisions between |revision_range_start| and
+ |revision_range_end|.
- return RunProcess(command)
+ Args:
+ revision_range_end: The SHA1 for the end of the range.
+ revision_range_start: The SHA1 for the beginning of the range.
- def GetRevisionList(self, revision_range_end, revision_range_start):
- """Retrieves a list of revisions between the bad revision and last known
- good revision."""
+ Returns:
+ A list of the revisions between |revision_range_start| and
+ |revision_range_end| (inc…
(message too large)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonhatch@chromium.org/12092033/18001
Message was sent while issue was closed.
Change committed as 180082 |
