Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7493016:
Added pre-fetching of builds. (Closed)
|
Created:
9 years, 5 months ago by szager Modified:
9 years, 3 months ago CC:
chromium-reviews, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUse threads to download builds in the background.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94810
Patch Set 1 #
Total comments: 29
Patch Set 2 : More documentation and codereview nits. #
Total comments: 16
Patch Set 3 : nits scratched #
Total comments: 5
Patch Set 4 : nits #
Total comments: 6
Patch Set 5 : nits #
Total comments: 2
Messages
Total messages: 14 (0 generated)
Unfortunately, the diffs are still pretty big...
http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode164 tools/bisect-builds.py:164: revlist = [int(x) for x in self.ParseDirectoryIndex()] map(int, self.ParseDirectoryIndex())? http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode172 tools/bisect-builds.py:172: pushd = os.getcwd() I know it's not yours, but could you rename this? |pushd| is a weird name. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode203 tools/bisect-builds.py:203: """Downloads and unzips revision |rev|""" Document |quit_event|. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode212 tools/bisect-builds.py:212: pass Log? http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode216 tools/bisect-builds.py:216: """Given a zipped revision, unzip it and run the test""" Comments require proper punctuation; here and elsewhere. To what does "the test" refer? http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode269 tools/bisect-builds.py:269: if profile is None: |if not profile:| is more idiomatic http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode285 tools/bisect-builds.py:285: pivot = bad/2 nit: Spaces around operators http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode287 tools/bisect-builds.py:287: zipfile = os.path.join(cwd, '%d-%s' % (rev, context.archive_name)) You do this format enough places that this could be a method on PathContext. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode292 tools/bisect-builds.py:292: print "iterating with good=%d bad=%d pivot=%d" % ( Use proper capitalization, but do you think this log really necessary/helpful? http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode295 tools/bisect-builds.py:295: # Pre-fetch next two possible pivots This block of code is hard to follow. It should have some high-level commentary. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode296 tools/bisect-builds.py:296: down_pivot = int((pivot - good) / 2) + good I don't really understand the naming rationale of "down" and "up". http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode298 tools/bisect-builds.py:298: if down_pivot != pivot and down_pivot != good : nit: no space before ':'. Here and elsewhere. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode301 tools/bisect-builds.py:301: down_zipfile = os.path.join(cwd, zipfile_base) This should really be a method on PathContext http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode334 tools/bisect-builds.py:334: down_event.set() # Kill the download of older revision nit: two spaces before comments
http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode164 tools/bisect-builds.py:164: revlist = [int(x) for x in self.ParseDirectoryIndex()] On 2011/07/25 16:13:07, rsesek wrote: > map(int, self.ParseDirectoryIndex())? Hmm... I am personally a fan of 'map', but my understanding is that it is unfashionable in the python world. This one's not worth fighting for, so I'll change it. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode172 tools/bisect-builds.py:172: pushd = os.getcwd() On 2011/07/25 16:13:07, rsesek wrote: > I know it's not yours, but could you rename this? |pushd| is a weird name. Done. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode203 tools/bisect-builds.py:203: """Downloads and unzips revision |rev|""" On 2011/07/25 16:13:07, rsesek wrote: > Document |quit_event|. Done. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode212 tools/bisect-builds.py:212: pass On 2011/07/25 16:13:07, rsesek wrote: > Log? This is not an actual error; this clause will be hit repeatedly during a normal run of the script, whenever a download is cancelled before it completes. It would probably be confusing if this generated noise. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode216 tools/bisect-builds.py:216: """Given a zipped revision, unzip it and run the test""" On 2011/07/25 16:13:07, rsesek wrote: > Comments require proper punctuation; here and elsewhere. To what does "the test" > refer? Done. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode269 tools/bisect-builds.py:269: if profile is None: On 2011/07/25 16:13:07, rsesek wrote: > |if not profile:| is more idiomatic Done. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode285 tools/bisect-builds.py:285: pivot = bad/2 On 2011/07/25 16:13:07, rsesek wrote: > nit: Spaces around operators Done. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode287 tools/bisect-builds.py:287: zipfile = os.path.join(cwd, '%d-%s' % (rev, context.archive_name)) On 2011/07/25 16:13:07, rsesek wrote: > You do this format enough places that this could be a method on PathContext. I created a local function for this; I prefer not to create this as a method on PathContext, since PathContext is really intended to manipulate URLs on the external archive, not for handling local paths. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode292 tools/bisect-builds.py:292: print "iterating with good=%d bad=%d pivot=%d" % ( On 2011/07/25 16:13:07, rsesek wrote: > Use proper capitalization, but do you think this log really necessary/helpful? Yeah, this is a bit TMI; I have removed it. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode295 tools/bisect-builds.py:295: # Pre-fetch next two possible pivots On 2011/07/25 16:13:07, rsesek wrote: > This block of code is hard to follow. It should have some high-level commentary. Added this to the comments for the Bisect function. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode296 tools/bisect-builds.py:296: down_pivot = int((pivot - good) / 2) + good On 2011/07/25 16:13:07, rsesek wrote: > I don't really understand the naming rationale of "down" and "up". Hopefully the high-level comments will clarify this; I have added a local comment as well. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode298 tools/bisect-builds.py:298: if down_pivot != pivot and down_pivot != good : On 2011/07/25 16:13:07, rsesek wrote: > nit: no space before ':'. Here and elsewhere. nit scratched. http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode301 tools/bisect-builds.py:301: down_zipfile = os.path.join(cwd, zipfile_base) On 2011/07/25 16:13:07, rsesek wrote: > This should really be a method on PathContext See above comment http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode334 tools/bisect-builds.py:334: down_event.set() # Kill the download of older revision On 2011/07/25 16:13:07, rsesek wrote: > nit: two spaces before comments nit scratched.
http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/7493016/diff/1/tools/bisect-builds.py#newcode287 tools/bisect-builds.py:287: zipfile = os.path.join(cwd, '%d-%s' % (rev, context.archive_name)) On 2011/07/25 17:39:04, szager wrote: > On 2011/07/25 16:13:07, rsesek wrote: > > You do this format enough places that this could be a method on PathContext. > > I created a local function for this; I prefer not to create this as a method on > PathContext, since PathContext is really intended to manipulate URLs on the > external archive, not for handling local paths. Not really true because it manipulates paths for the archives. But what you've done is fine because it's local to this function. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:205: if quit_event and quit_event.is_set(): nit: capitalization http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:274: nit: capitalization http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:275: revlist = context.GetRevList() über-nit: this file uses single spaces between sentences. You should also avoid the use of the first person in comments. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:295: # Pre-fetch next two possible pivots Can you break this after the comma? That's probably a little cleaner looking. Prefix with _ to make it clear it's local? http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:345: up_event.set() # Kill download of newer revision nit: full-stop comments. Here and elsewhere. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:353: except SystemExit: This block needs a comment. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:354: for f in [down_zipfile, up_zipfile]: Could the work in these two branches be refactored out into a function? http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:358: pass Would it make sense to reuse the threads so you don't have to create and join them? Or do you think that is more work than it's worth?
New patch, more comments addressed. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:205: @param rev The chromium revision number/tag to download. On 2011/07/26 21:00:26, rsesek wrote: > nit: capitalization Done. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:274: Threading is used to fetch chromium revisions in the background, speeding up On 2011/07/26 21:00:26, rsesek wrote: > nit: capitalization Done. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:275: the user's experience. For example, suppose the bounds of the search are On 2011/07/26 21:00:26, rsesek wrote: > über-nit: this file uses single spaces between sentences. You should also avoid > the use of the first person in comments. Done. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:295: GetDownloadPath = lambda rev: os.path.join(cwd, '%d-%s' % ( On 2011/07/26 21:00:26, rsesek wrote: > Can you break this after the comma? That's probably a little cleaner looking. > Prefix with _ to make it clear it's local? Done. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:345: # Run test on the pivot revision On 2011/07/26 21:00:26, rsesek wrote: > nit: full-stop comments. Here and elsewhere. Done. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:353: try: On 2011/07/26 21:00:26, rsesek wrote: > This block needs a comment. Done. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:354: if predicate(rev, status, stdout, stderr): On 2011/07/26 21:00:26, rsesek wrote: > Could the work in these two branches be refactored out into a function? Perhaps, but it would have to be one of those functions with lots of arguments and just a few lines of code. If I *really* wanted to abstract out this code, I would probably create a new class which encapsulates down_thread, down_event, down_pivot, etc. Could go either way, but I tend to think that would be overkill. http://codereview.chromium.org/7493016/diff/3001/tools/bisect-builds.py#newco... tools/bisect-builds.py:358: down_thread.join() On 2011/07/26 21:00:26, rsesek wrote: > Would it make sense to reuse the threads so you don't have to create and join > them? Or do you think that is more work than it's worth? I don't think there's any measurable performance gain to be had there.
http://codereview.chromium.org/7493016/diff/8001/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/7493016/diff/8001/tools/bisect-builds.py#newco... tools/bisect-builds.py:231: # Run the test All comments must have proper punctuation. Here and throughout this change. s/test/build http://codereview.chromium.org/7493016/diff/8001/tools/bisect-builds.py#newco... tools/bisect-builds.py:296: '%d-%s' % (rev, context.archive_name)) nit: indent 4 for line wraps
More nits. http://codereview.chromium.org/7493016/diff/8001/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/7493016/diff/8001/tools/bisect-builds.py#newco... tools/bisect-builds.py:231: # Run the test On 2011/07/29 19:19:47, rsesek wrote: > All comments must have proper punctuation. Here and throughout this change. > > s/test/build Done. http://codereview.chromium.org/7493016/diff/8001/tools/bisect-builds.py#newco... tools/bisect-builds.py:296: '%d-%s' % (rev, context.archive_name)) On 2011/07/29 19:19:47, rsesek wrote: > nit: indent 4 for line wraps Done.
LGTM with nits. Excited to see this in action! http://codereview.chromium.org/7493016/diff/8001/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/7493016/diff/8001/tools/bisect-builds.py#newco... tools/bisect-builds.py:231: # Run the test On 2011/07/29 20:22:55, szager1 wrote: > On 2011/07/29 19:19:47, rsesek wrote: > > All comments must have proper punctuation. Here and throughout this change. > > > > s/test/build > > Done. Nope. Please comb the CL when a reviewer says "throughout." http://codereview.chromium.org/7493016/diff/12001/tools/bisect-builds.py#newc... tools/bisect-builds.py:225: # Create a temp directory and unzip the revision into it . http://codereview.chromium.org/7493016/diff/12001/tools/bisect-builds.py#newc... tools/bisect-builds.py:301: if len(revlist) < 2: # Don't have enough builds to bisect . http://codereview.chromium.org/7493016/diff/12001/tools/bisect-builds.py#newc... tools/bisect-builds.py:361: down_event.set() # Kill the download of older revision here http://codereview.chromium.org/7493016/diff/12001/tools/bisect-builds.py#newc... tools/bisect-builds.py:366: up_thread.join() # Wait for newer revision to finish downloading here http://codereview.chromium.org/7493016/diff/12001/tools/bisect-builds.py#newc... tools/bisect-builds.py:372: up_event.set() # Kill download of newer revision . http://codereview.chromium.org/7493016/diff/12001/tools/bisect-builds.py#newc... tools/bisect-builds.py:377: down_thread.join() # Wait for older revision to finish downloading .
Change committed as 94810
http://codereview.chromium.org/7493016/diff/16001/tools/bisect-builds.py File tools/bisect-builds.py (right): http://codereview.chromium.org/7493016/diff/16001/tools/bisect-builds.py#newc... tools/bisect-builds.py:366: up_thread.join() # Wait for newer revision to finish downloading. Can you make it so that this prints progress? I often bisect things on the shuttle, which has a pretty slow wireless connection, and this patch seems to have regressed progress output.
Shouldn't be too hard; I'll work on it. Stefan On Thu, Aug 4, 2011 at 3:25 PM, <rsesek@chromium.org> wrote: > > http://codereview.chromium.**org/7493016/diff/16001/tools/** > bisect-builds.py<http://codereview.chromium.org/7493016/diff/16001/tools/bisect-builds.py> > File tools/bisect-builds.py (right): > > http://codereview.chromium.**org/7493016/diff/16001/tools/** > bisect-builds.py#newcode366<http://codereview.chromium.org/7493016/diff/16001/tools/bisect-builds.py#newcode366> > tools/bisect-builds.py:366: up_thread.join() # Wait for newer revision > to finish downloading. > On 2011/08/04 20:47:12, Nico wrote: > >> Can you make it so that this prints progress? I often bisect things on >> > the > >> shuttle, which has a pretty slow wireless connection, and this patch >> > seems to > >> have regressed progress output. >> > > The reason I let this get removed is I wasn't sure how two builds being > downloaded at once would look/work with the \r flushing. I agree it'd be > nice-to-have, though not at the expense of the multi-build downloading. > > > http://codereview.chromium.**org/7493016/<http://codereview.chromium.org/7493... >
Why was http://codereview.chromium.org/7468020/ bulldozed by this change? Has the existing download server been updated to store more builds?
On Thu, Sep 8, 2011 at 11:46 AM, <jbates@chromium.org> wrote: > Why was http://codereview.chromium.org/7468020/ bulldozed by this change? > Has > the existing download server been updated to store more builds? > > > http://codereview.chromium.org/7493016/ > http://build.chromium.org/f/chromium/snapshots is obsolete; snapshots now go directly to google storage. So, there's no distinction to be made between recent and old builds. Stefan
> http://build.chromium.org/f/chromium/snapshots is obsolete; snapshots > now go directly to google storage. So, there's no distinction to be > made between recent and old builds. Ah, great! Too bad though, since the chromium.org download speeds were faster. But your threaded downloads should hopefully solve that. |
