Refactored bisect results dicts into a separate class

Refactored bisect results dicts into a separate class

R=qyearsley@chromium.org

Committed: https://crrev.com/cdb492dbbabe59c2aec3980804e500d1a0618ac2
Cr-Commit-Position: refs/heads/master@{#297006}

[Sergiy Byelozyorov, 2014-09-18 15:57:12]

PTAL

[Sergiy Byelozyorov, 2014-09-18 15:57:22]

sergiyb@chromium.org changed reviewers:
+ ojan@chromium.org

[Sergiy Byelozyorov, 2014-09-18 15:57:31]

Added ojan to the reviewers.

[qyearsley, 2014-09-19 07:35:16]

This CL looks like a step in the right direction, although it goes without
saying that the GetResultsDict method is still huge.

(Also: the state of the bisect job while the job is in progress is still stored
in a dict called revision_data. The information about each test results for each
revision are still stored in there. I think we could make make the operations
involving bisect job state and revision data clearer by creating classes for
these things.)

https://codereview.chromium.org/554283003/diff/100001/tools/auto_bisect/bisec...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/554283003/diff/100001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:875: self._bisect_perf_metrics =
bisect_perf_metrics
Initial thought: BisectPerformanceMetrics is a massive class with many methods
and properties; if possible, I think we want to avoid using it here.

If "ChangeToDepotWorkingDirectory" could be made into a top-level function, then
we wouldn't need to have keep a BisectPerformanceMetrics instance here right?

https://codereview.chromium.org/554283003/diff/100001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:921: """Returns a dictionary with
the following fields
The first line of a docstring is generally a self-contained sentence which is
useful for skimming. You could instead write something like:

"""Prepares and returns information about the final results as a dict.

This dictionary will have the following fields:
  ...
"""

https://codereview.chromium.org/554283003/diff/100001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:937: bisects.
I think this formatting might look better if line continuations were indented by
four spaces instead of aligned with the first line.

[Sergiy Byelozyorov, 2014-09-19 13:36:22]

Addressed comments. I'll extract revision_data and simplify GetRevisionDict in a
follow-up CL.

https://codereview.chromium.org/554283003/diff/100001/tools/auto_bisect/bisec...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/554283003/diff/100001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:875: self._bisect_perf_metrics =
bisect_perf_metrics
On 2014/09/19 07:35:16, qyearsley wrote:
> Initial thought: BisectPerformanceMetrics is a massive class with many methods
> and properties; if possible, I think we want to avoid using it here.
> 
> If "ChangeToDepotWorkingDirectory" could be made into a top-level function,
then
> we wouldn't need to have keep a BisectPerformanceMetrics instance here right?

Done.

https://codereview.chromium.org/554283003/diff/100001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:921: """Returns a dictionary with
the following fields
On 2014/09/19 07:35:16, qyearsley wrote:
> The first line of a docstring is generally a self-contained sentence which is
> useful for skimming. You could instead write something like:
> 
> """Prepares and returns information about the final results as a dict.
> 
> This dictionary will have the following fields:
>   ...
> """ 

Done.

https://codereview.chromium.org/554283003/diff/100001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:937: bisects.
On 2014/09/19 07:35:16, qyearsley wrote:
> I think this formatting might look better if line continuations were indented
by
> four spaces instead of aligned with the first line.

Done.

[ojan, 2014-09-19 18:17:00]

Please add basic tests for the code you're moving into classes. One of the big
benefits of a patch like this one is that it enables better test coverage. Since
you're just moving code around, I don't expect you to get 100% test coverage,
but get at least one very basic test of each function in the new classes if you
can. That way, as people modify those functions/classes in the future, it's
natural for them to add a test as well. Overtime, we approach a codebase that
has good test coverage.

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:871: class BisectResults(object):
All these classes should do in their own files. It makes code management a lot
more sane. The only exception for this is truly trivial classes that are just
structs with no code.

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:872: """This class holds the results
of the bisect."""
+1 to removing comments like this. This doesn't provide any value over the name
of the class. Please omit comments that just repeat what the class or function
name + argument names say.

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:924: A dictionary with the following
fields
In my experience, documentation like this almost immediately gets stale and
starts being incorrect as the code changes but the documentation isn't updated.
But I don't feel strongly about this. If it were magically kept up to date it
would be useful. I think there might be other folks on infra team who like this
sort of documentation.

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:1086: class DepotManager(object):
Don't call it a manager. A manager can literally do anything. Nearly every class
is a manager of something. How about...DepotDirectoryChanger? That's not a great
name, but it's at least more clear what this class does.

[Sergiy Byelozyorov, 2014-09-22 19:30:13]

Will add tests tomorrow.

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:871: class BisectResults(object):
On 2014/09/19 18:17:00, ojan-only-code-yellow-reviews wrote:
> All these classes should do in their own files. It makes code management a lot
> more sane. The only exception for this is truly trivial classes that are just
> structs with no code.

Done.

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:872: """This class holds the results
of the bisect."""
On 2014/09/19 18:17:00, ojan-only-code-yellow-reviews wrote:
> +1 to removing comments like this. This doesn't provide any value over the
name
> of the class. Please omit comments that just repeat what the class or function
> name + argument names say.

Done.

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:924: A dictionary with the following
fields
On 2014/09/19 18:17:00, ojan-only-code-yellow-reviews wrote:
> In my experience, documentation like this almost immediately gets stale and
> starts being incorrect as the code changes but the documentation isn't
updated.
> But I don't feel strongly about this. If it were magically kept up to date it
> would be useful. I think there might be other folks on infra team who like
this
> sort of documentation.

It actually took me a while to gather this info and compile it here, so I think
this is useful. While some fields may be added or removed it's good to have a
short description for at least a subset of them rather than having to read the
code to understand the purpose of a given field.

Also it will get simpler when I'll separate RevisionData into a separate class.

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:1086: class DepotManager(object):
On 2014/09/19 18:17:00, ojan-only-code-yellow-reviews wrote:
> Don't call it a manager. A manager can literally do anything. Nearly every
class
> is a manager of something. How about...DepotDirectoryChanger? That's not a
great
> name, but it's at least more clear what this class does.

How about DepotDirectoryRegistry? I'd like to keep the name generic, because
this class not only changes directories (for this os.chdir would be enough), but
also keeps an up-to-date depot-to-directory mapping.

[Sergiy Byelozyorov, 2014-09-23 18:35:23]

Added tests. PTAL & CQ

[Sergiy Byelozyorov, 2014-09-24 13:43:12]

Correction: DO NOT CQ this. It still depends on another CL to be checked in
first.

[qyearsley, 2014-09-24 16:54:45]

On 2014/09/24 13:43:12, Sergiy Byelozyorov wrote:
> Correction: DO NOT CQ this. It still depends on another CL to be checked in
> first.

Note: I haven't submitted the CL that this depends on
(codereview.chromium.org/564663002) because I haven't figured out the error that
I get when I try it out.

(Log with error:
http://build.chromium.org/p/tryserver.chromium.perf/builders/win_8_perf_bisec...)

I'm wondering, if you you try it out, do you get the same error? Here are some
instructions I just wrote up for starting a try job:
https://sites.google.com/a/google.com/chromeperf/auto-bisect/hacking-on-auto-...

[Sergiy Byelozyorov, 2014-09-25 13:29:42]

I can't even get the sync to finish correctly:

http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisec...
http://build.chromium.org/p/tryserver.chromium.perf/builders/win_8_perf_bisec...
http://build.chromium.org/p/tryserver.chromium.perf/builders/win_8_perf_bisec...

When running locally I get another error:

Error: An error occurred while building and running the 'bad' reference value.
The bisect cannot continue without a working 'bad' revision to start from.

Error: Failed to sync: [fe72eb2e7a7e394d020f580f76f8840410dfcdf2]

[Sergiy Byelozyorov, 2014-09-25 13:33:55]

Also after running it locally, the repo was left in the  rebase state with the
following conflicts:

$ git diff
diff --cc tools/auto_bisect/README
index d43222c,c1a88e7..0000000
--- a/tools/auto_bisect/README
+++ b/tools/auto_bisect/README
@@@ -9,15 -9,10 +9,25 @@@ Documentation
   
http://www.chromium.org/developers/tree-sheriffs/perf-sheriffs/bisecting-perf...
  
  Overview of bisect-related files in src/tools:
++<<<<<<< HEAD
 +  run-bisect-perf-regression.py
 +     -- the script used to kick off a normal performance regression bisect
job.
 +  run-bisect-perf-regression.cfg
 +     -- this file contains parameters for a bisect job, and is read by other
 +        modules including run-bisect-perf-regresion.py.
 +  run-bisect-manual-test.py
 +     -- a script which is used to manually bisect regressions; this also
 +        depends on bisect-perf-gression.py.
 +  bisect-perf-regression.py
 +     -- the main module which the others depend on.
 +  bisect-manual-test.py
 +     -- a helper module used when manually bisect regressions.
++=======
+   run-bisect-perf-regression.py -- used to kick off a bisect job
+   prepare-bisect-perf-regression.py -- run before the above to prepare the
repo
+   run-bisect-manual-test.py -- used to manually bisect
+   bisect-manual-test.py -- helper module used by run-bisect-manual-test.py
+   auto_bisect/bisect.cfg -- config parameters for a bisect job
+   run-perf-test.cfg -- config parameters running a perf test once
+ 
++>>>>>>> patch from issue 564663002
diff --cc tools/auto_bisect/bisect_perf_regression.py
index 363aa1b,22dccc1..0000000
--- a/tools/auto_bisect/bisect_perf_regression.py
+++ b/tools/auto_bisect/bisect_perf_regression.py
@@@ -13,21 -13,19 +13,21 @@@ suspected to occur as a result of WebKi
  further bisect changes to those depots and attempt to narrow down the revision
  range.
  
 -Example usage using SVN revisions:
 +
 +An example usage (using svn cl's):
  
- ./tools/bisect-perf-regression.py -c\
+ ./tools/bisect_perf_regression.py -c\
    "out/Release/performance_ui_tests
--gtest_filter=ShutdownTest.SimpleUserQuit"\
    -g 168222 -b 168232 -m shutdown/simple-user-quit
  
 -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
 +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.
  
 -Example usage using git hashes:
 +
 +An example usage (using git hashes):
  
- ./tools/bisect-perf-regression.py -c\
+ ./tools/bisect_perf_regression.py -c\
    "out/Release/performance_ui_tests
--gtest_filter=ShutdownTest.SimpleUserQuit"\
    -g 1f6e67861535121c5c819c16a666f2436c207e7b\
    -b b732f23b4f81c382db0b23b9035f3dadc7d925bb\
@@@ -49,13 -47,15 +49,23 @@@ import sy
  import time

[qyearsley, 2014-09-25 16:54:30]

On 2014/09/25 13:33:55, Sergiy Byelozyorov wrote:
> Also after running it locally, the repo was left in the  rebase state with the
> following conflicts:
> 

Darn, that's annoying. I got the same thing last time I tried to run it locally,
when I passed "-w ~" instead of "-w ." so that (I believe) it would check out
some repo in ~/bisect...

Meanwhile, I'm still trying to debug the failure I get when I run it on the
bisect bots as a try job.

[ojan, 2014-09-26 17:34:30]

lgtm

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/554283003/diff/200001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:1086: class DepotManager(object):
On 2014/09/22 19:30:12, Sergiy Byelozyorov wrote:
> On 2014/09/19 18:17:00, ojan-only-code-yellow-reviews wrote:
> > Don't call it a manager. A manager can literally do anything. Nearly every
> class
> > is a manager of something. How about...DepotDirectoryChanger? That's not a
> great
> > name, but it's at least more clear what this class does.
> 
> How about DepotDirectoryRegistry? I'd like to keep the name generic, because
> this class not only changes directories (for this os.chdir would be enough),
but
> also keeps an up-to-date depot-to-directory mapping.

SGTM. That's much more clear.

https://codereview.chromium.org/554283003/diff/240001/tools/auto_bisect/bisec...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/554283003/diff/240001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression.py:835: """Manages depots (code
repositories) and their directories."""
IMO, this comment doesn't say anything the name of the class already does.

[Sergiy Byelozyorov, 2014-09-26 18:59:56]

The CQ bit was checked by sergiyb@chromium.org

[commit-bot: I haz the power, 2014-09-26 19:01:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/554283003/280001

[Sergiy Byelozyorov, 2014-09-26 19:01:20]

The CQ bit was unchecked by sergiyb@chromium.org

[Sergiy Byelozyorov, 2014-09-26 19:08:13]

The CQ bit was checked by sergiyb@chromium.org

[commit-bot: I haz the power, 2014-09-26 19:09:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/554283003/280001

[commit-bot: I haz the power, 2014-09-26 19:39:53]

Committed patchset #15 (id:280001) as 4612422c74d7ac1783dab4f3cf79334995fbae88

[commit-bot: I haz the power, 2014-09-26 19:40:24]

Patchset 15 (id:??) landed as
https://crrev.com/cdb492dbbabe59c2aec3980804e500d1a0618ac2
Cr-Commit-Position: refs/heads/master@{#297006}