Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10939023:
Adding gather_stats (based on bisect-builds.py) to gather benchmarking (Closed)
|
Created:
8 years, 3 months ago by hartmanng Modified:
7 years, 4 months ago Reviewers:
Ian Vollick CC:
chromium-reviews, pam+watch_chromium.org, apatrick_chromium, Robert Sesek Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding gather_stats (based on bisect-builds.py) to gather benchmarking
statistics from a series of chrome builds.
BUG=151962
Patch Set 1 #
Total comments: 16
Patch Set 2 : #Patch Set 3 : Reverted bisect-builds.py to original name #
Total comments: 6
Patch Set 4 : #Messages
Total messages: 14 (0 generated)
https://chromiumcodereview.appspot.com/10939023/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): https://chromiumcodereview.appspot.com/10939023/diff/1/tools/bisect-builds.py... tools/bisect-builds.py:304: outdir=None): Why is tempdir a parameter? It doesn't look like gather_stats.py makes use of the ability to specify this. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/bisect-builds.py... tools/bisect-builds.py:334: ferr.close() It looks like we shutil.rmtree after every call to RunRevision. Why not put that code there? https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats.py File tools/gpu/gather_stats.py (right): https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:12: it will continuing the process. Does this script do all the guessing and asking that is claimed here? I'd ditch the last sentence, and in the sentence previous I'd s/opening Chromium for you/gathers stats for each revision/ https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:21: sys.path.insert(0, "..") Does this make an assumption about where the script is run from? https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:57: # Figure out our bookends and first pivot point; fetch the pivot revision. Should avoid bisect-specific language now. Pivot is out, maybe pivot -> index? https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:67: while zipfile and pivot < len(revlist): Why not 'for rev in revlist:'? Also, isn't pivot currently the index of the revision we're prefetching? It looks like we won't enter this list and test the last revision since the prefetch index will be too high. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:116: 'Perform binary search on the snapshot builds.\n' This text needs to be updated. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:134: help = 'The revision to start from. Default is 0.') 0's a rough starting revision! It would be nice if we could also specify a time delta, like a month, and the default was going back one month.
I think I've addressed all your comments, could you take another look whenever you have a minute before I send it to owners? https://chromiumcodereview.appspot.com/10939023/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): https://chromiumcodereview.appspot.com/10939023/diff/1/tools/bisect-builds.py... tools/bisect-builds.py:304: outdir=None): On 2012/09/18 17:46:32, vollick wrote: > Why is tempdir a parameter? It doesn't look like gather_stats.py makes use of > the ability to specify this. Done. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/bisect-builds.py... tools/bisect-builds.py:334: ferr.close() On 2012/09/18 17:46:32, vollick wrote: > It looks like we shutil.rmtree after every call to RunRevision. Why not put that > code there? Done. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats.py File tools/gpu/gather_stats.py (right): https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:12: it will continuing the process. On 2012/09/18 17:46:32, vollick wrote: > Does this script do all the guessing and asking that is claimed here? I'd ditch > the last sentence, and in the sentence previous I'd s/opening Chromium for > you/gathers stats for each revision/ Done. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:21: sys.path.insert(0, "..") On 2012/09/18 17:46:32, vollick wrote: > Does this make an assumption about where the script is run from? Done. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:57: # Figure out our bookends and first pivot point; fetch the pivot revision. On 2012/09/18 17:46:32, vollick wrote: > Should avoid bisect-specific language now. Pivot is out, maybe pivot -> index? Done. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:67: while zipfile and pivot < len(revlist): On 2012/09/18 17:46:32, vollick wrote: > Why not 'for rev in revlist:'? > > Also, isn't pivot currently the index of the revision we're prefetching? It > looks like we won't enter this list and test the last revision since the > prefetch index will be too high. Yes you're right, it didn't test the last revision (Fixed). However, changing the loop to a "for in" style loop makes it difficult to perform prefetching since we need variables pointing at 2 separate points in revlist at the same time. We could do "for next_rev in revlist[1:}:" since we already have rev pointing to revlist[0] (and keep updating rev = next_rev at the end of each iteration), but then the loop will again end too early to execute the last prefetched revision unless we add an extra call to RunRevision after the loop. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:116: 'Perform binary search on the snapshot builds.\n' On 2012/09/18 17:46:32, vollick wrote: > This text needs to be updated. Done. https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... tools/gpu/gather_stats.py:134: help = 'The revision to start from. Default is 0.') On 2012/09/18 17:46:32, vollick wrote: > 0's a rough starting revision! It would be nice if we could also specify a time > delta, like a month, and the default was going back one month. I agree, but do you know a good way to get dates of the revisions? The current bisect_builds.py script's revlist source (eg http://commondatastorage.googleapis.com/chromium-browser-snapshots/?delimiter... ) doesn't seem to have easily-accessible datestamps. For now I've put a revision delta instead of a time delta.
Cool, LGTM. I think it's ready for OWNERS review. On 2012/09/19 02:17:55, hartmanng wrote: > I think I've addressed all your comments, could you take another look whenever > you have a minute before I send it to owners? > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/bisect-builds.py > File tools/bisect-builds.py (right): > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/bisect-builds.py... > tools/bisect-builds.py:304: outdir=None): > On 2012/09/18 17:46:32, vollick wrote: > > Why is tempdir a parameter? It doesn't look like gather_stats.py makes use of > > the ability to specify this. > > Done. > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/bisect-builds.py... > tools/bisect-builds.py:334: ferr.close() > On 2012/09/18 17:46:32, vollick wrote: > > It looks like we shutil.rmtree after every call to RunRevision. Why not put > that > > code there? > > Done. > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats.py > File tools/gpu/gather_stats.py (right): > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... > tools/gpu/gather_stats.py:12: it will continuing the process. > On 2012/09/18 17:46:32, vollick wrote: > > Does this script do all the guessing and asking that is claimed here? I'd > ditch > > the last sentence, and in the sentence previous I'd s/opening Chromium for > > you/gathers stats for each revision/ > > Done. > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... > tools/gpu/gather_stats.py:21: sys.path.insert(0, "..") > On 2012/09/18 17:46:32, vollick wrote: > > Does this make an assumption about where the script is run from? > > Done. > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... > tools/gpu/gather_stats.py:57: # Figure out our bookends and first pivot point; > fetch the pivot revision. > On 2012/09/18 17:46:32, vollick wrote: > > Should avoid bisect-specific language now. Pivot is out, maybe pivot -> index? > > Done. > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... > tools/gpu/gather_stats.py:67: while zipfile and pivot < len(revlist): > On 2012/09/18 17:46:32, vollick wrote: > > Why not 'for rev in revlist:'? > > > > Also, isn't pivot currently the index of the revision we're prefetching? It > > looks like we won't enter this list and test the last revision since the > > prefetch index will be too high. > > Yes you're right, it didn't test the last revision (Fixed). However, changing > the loop to a "for in" style loop makes it difficult to perform prefetching > since we need variables pointing at 2 separate points in revlist at the same > time. > > We could do "for next_rev in revlist[1:}:" since we already have rev pointing to > revlist[0] (and keep updating rev = next_rev at the end of each iteration), but > then the loop will again end too early to execute the last prefetched revision > unless we add an extra call to RunRevision after the loop. > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... > tools/gpu/gather_stats.py:116: 'Perform binary search on the snapshot builds.\n' > On 2012/09/18 17:46:32, vollick wrote: > > This text needs to be updated. > > Done. > > https://chromiumcodereview.appspot.com/10939023/diff/1/tools/gpu/gather_stats... > tools/gpu/gather_stats.py:134: help = 'The revision to start from. Default is > 0.') > On 2012/09/18 17:46:32, vollick wrote: > > 0's a rough starting revision! It would be nice if we could also specify a > time > > delta, like a month, and the default was going back one month. > > I agree, but do you know a good way to get dates of the revisions? The current > bisect_builds.py script's revlist source (eg > http://commondatastorage.googleapis.com/chromium-browser-snapshots/?delimiter... > ) doesn't seem to have easily-accessible datestamps. For now I've put a revision > delta instead of a time delta.
Please take a look.
Sorry about not commenting on this yesterday. I haven't looked at the code closely yet. However, I'm somewhat concerned with the file move. I suppose that's because you can't have '-' in a module name. bisect-builds.py is used by many more people than just chromium developers. We often link folks to http://www.chromium.org/developers/bisect-builds-py , which suggests downloading just the python script. I guess we could change the link on that page to bisect_builds.py (which is still self-contained), and keep tools/bisect-builds.py aroudn as a trampoline that just forwards to bisect_builds.py -- but gather_stats.py won't be self-contained in that case. I'm not sure if that's important. Is it possible to just add the gather_stats stuff to bisect-builds? Then no renaming is necessary. (I don't quite understand the motivation behind having this script -- having a bug that explains what your larger goal is and mentioning that in the BUG= line is usually helpful for reviews for that reason.)
On 2012/09/21 03:59:10, Nico wrote: > Sorry about not commenting on this yesterday. > > I haven't looked at the code closely yet. However, I'm somewhat concerned with > the file move. I suppose that's because you can't have '-' in a module name. > > bisect-builds.py is used by many more people than just chromium developers. We > often link folks to http://www.chromium.org/developers/bisect-builds-py , which > suggests downloading just the python script. I guess we could change the link on > that page to bisect_builds.py (which is still self-contained), and keep > tools/bisect-builds.py aroudn as a trampoline that just forwards to > bisect_builds.py -- but gather_stats.py won't be self-contained in that case. > I'm not sure if that's important. > > Is it possible to just add the gather_stats stuff to bisect-builds? Then no > renaming is necessary. > > (I don't quite understand the motivation behind having this script -- having a > bug that explains what your larger goal is and mentioning that in the BUG= line > is usually helpful for reviews for that reason.) Makes sense, I reverted bisect-builds.py back to its original name. For now I've added a workaround in gather_stats.py to import bisect-builds.py (might be a bit cleaner as separate files), but if you think it's better to merge them into one file I can do that. I also added a bug with a small summary of our intentions with this script, if you want more info don't hesitate to ask. PTAL
FWIW, I think these scripts should remain separate. Adding linear scanning to bisect-builds literally means it's no longer bisect-builds. Thanks for keeping the name the same, though it means a less-pretty import. bisect-builds also needs to remain a single file for ease of use and installation for non-eng people. http://codereview.chromium.org/10939023/diff/12001/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/10939023/diff/12001/tools/bisect-builds.py#new... tools/bisect-builds.py:300: """Given a zipped revision, unzip it and run the test.""" Can you properly document this function's params? It's got enough of them that it probably should be done. http://codereview.chromium.org/10939023/diff/12001/tools/bisect-builds.py#new... tools/bisect-builds.py:325: 'w') This should probably align with the 'o' in |os| since it's the second param to open(), not to join(). http://codereview.chromium.org/10939023/diff/12001/tools/gpu/gather_stats.py File tools/gpu/gather_stats.py (right): http://codereview.chromium.org/10939023/diff/12001/tools/gpu/gather_stats.py#... tools/gpu/gather_stats.py:30: def gather_stats(platform, Chromium style should be GatherStats.
http://codereview.chromium.org/10939023/diff/12001/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/10939023/diff/12001/tools/bisect-builds.py#new... tools/bisect-builds.py:300: """Given a zipped revision, unzip it and run the test.""" On 2012/09/24 17:52:30, rsesek wrote: > Can you properly document this function's params? It's got enough of them that > it probably should be done. Done. http://codereview.chromium.org/10939023/diff/12001/tools/bisect-builds.py#new... tools/bisect-builds.py:325: 'w') On 2012/09/24 17:52:30, rsesek wrote: > This should probably align with the 'o' in |os| since it's the second param to > open(), not to join(). Done. http://codereview.chromium.org/10939023/diff/12001/tools/gpu/gather_stats.py File tools/gpu/gather_stats.py (right): http://codereview.chromium.org/10939023/diff/12001/tools/gpu/gather_stats.py#... tools/gpu/gather_stats.py:30: def gather_stats(platform, On 2012/09/24 17:52:30, rsesek wrote: > Chromium style should be GatherStats. Done.
Pretty nice. Lets make sure rsesek/nico/whomever is happy with the changes to bisect_builds and then I can do a final pass.
(And, you knew it was coming: you need a tools/gpu/gather_stats_unittest.py)
Should we close this since its idle?
On 2012/11/06 04:29:06, nduca wrote: > Should we close this since its idle? I'd love for this to happen eventually -- I think it'd be really useful for tracking down regressions -- and I'm worried that if we close it, it'll never happen. Can we maybe keep this open and lower the priority of the associated bug.
Seems fine. I would suggest removing the reivewers from the issue so its not spamming the review queue.
On 2012/11/06 16:20:13, nduca wrote: > Seems fine. I would suggest removing the reivewers from the issue so its not > spamming the review queue. -reviewers (vollick, thakis, Nico, rsesek, nduca) |
