Add -j option to gclient to run parallel updates

Add -j option to gclient to run parallel updates

On a chromeos checkout, -j 10 brings down null sync time from 2 minutes to 16 seconds
Currently -j may break some assumptions about ordering of commands (which may be fine for some clients, but not all), so it's not on by default.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44869

[M-A Ruel, 2010-04-10 01:16:58]

I like the idea but even if it's optional, it could still be a breaking change,
I'm especially worried about --revision flag with multiple entries in .gclient,
I'd want you to verify it's correctly working. It's the kind of change I prefer
to be checked-in over the weekend, never fun to force a full checkout on an
hundred slaves during a work day. :)

The other things that worries me is when a svn ask for credentials. It could
become a mess if you're synching from 2 svn repos that asks for credentials. You
should clean your svn key cache and see what happens.

http://codereview.chromium.org/1640001/diff/1/2
File gclient.py (right):

http://codereview.chromium.org/1640001/diff/1/2#newcode316
gclient.py:316: class ThreadPool:
I'd prefer to have that class in gclients_utils.py if you don't mind.

http://codereview.chromium.org/1640001/diff/1/2#newcode782
gclient.py:782: #print "Running %s in %s to %s %s" % (command, path, url,
revision)
Use logging.debug()

http://codereview.chromium.org/1640001/diff/1/2#newcode836
gclient.py:836: thread_pool.Start()
I'd prefer:

thread_pool.Start()
try:
  ...
finally:
  thread_pool.WaitJobs()

http://codereview.chromium.org/1640001/diff/1/2#newcode854
gclient.py:854: command, args, file_list))
The behavior is slightly different since options.revision is not set to None
anymore. I'm just wondering if it affects "gclient sync --revision src@123".

http://codereview.chromium.org/1640001/diff/1/2#newcode873
gclient.py:873: pass
???

http://codereview.chromium.org/1640001/diff/1/3
File tests/gclient_test.py (right):

http://codereview.chromium.org/1640001/diff/1/3#newcode304
tests/gclient_test.py:304: for key in dir(options):
There's an error in your code. Here's a better version:

a = [i for i in dir(options) if not i.startswith('_')]
b = [i for i in dir(others) if not i.startswith('_')]
if a != b:
  return False
try:
  for key in a:
    if getattr(a, key) != getattr(b, key):
      return False
  return True
except AttributeError:
  return False

You can rename a and b :)

[piman, 2010-04-10 01:48:44]

Other than noted, I will do the suggested changes. I'll do more testing next
week, including the svn credential thing.
I already did some fair amount of testing on both ChromeOS and Chrome checkouts,
but I hadn't tried that.

I think with -j n (n>=2), interactive prompts could be a problem, but I don't
have a good idea how to solve it (given that prompts come from third-party
programs) other than saying it's not a problem with n=1. And I think it's
acceptable to be able to optimize the common case, and have the user fall back
to -j1 when things get hairy.

http://codereview.chromium.org/1640001/diff/1/2
File gclient.py (right):

http://codereview.chromium.org/1640001/diff/1/2#newcode854
gclient.py:854: command, args, file_list))
On 2010/04/10 01:16:58, Marc-Antoine Ruel wrote:
> The behavior is slightly different since options.revision is not set to None
> anymore. I'm just wondering if it affects "gclient sync --revision src@123".

self._options.revision is never set by the command-line options (they set
revisions -- plural), it is only set in the code.
In the old code it is only used when passed to the SCM command, in which case it
is always overwritten before.
With my change they are never set, correct, but I don't think it impacts
anything. By the way, I ran the tests (after updating to compare the options
contents instead of their pointer) and they pass.
I can add an explicit self._options.revision = None, but I don't think it's
useful

http://codereview.chromium.org/1640001/diff/1/2#newcode873
gclient.py:873: pass
On 2010/04/10 01:16:58, Marc-Antoine Ruel wrote:
> ???

file_list_dict[name] is only set above if several conditions are present
(run_scm and url). Instead of duplicating the code to run the exact same tests,
I figured it's easier and less fragile to check for existence of the key this
way.

[M-A Ruel, 2010-04-10 01:59:41]

lgtm

[piman, 2010-04-12 23:51:57]

PTAL. If you'd rather wait on a week-end for the checkin, let me know. We don't
have any canary system for gclient, do we ?
I'll try to motivate a couple of co-workers for a bit of field testing too.

I tested with wiping the svn credentials. Surprisingly, I was able to log on the
command line when using -j10, even though the svn messages are somewhat
confusing. It works with no -j option as well, expectedly.

http://codereview.chromium.org/1640001/diff/1/2
File gclient.py (right):

http://codereview.chromium.org/1640001/diff/1/2#newcode316
gclient.py:316: class ThreadPool:
On 2010/04/10 01:16:58, Marc-Antoine Ruel wrote:
> I'd prefer to have that class in gclients_utils.py if you don't mind.

Done.

http://codereview.chromium.org/1640001/diff/1/2#newcode782
gclient.py:782: #print "Running %s in %s to %s %s" % (command, path, url,
revision)
On 2010/04/10 01:16:58, Marc-Antoine Ruel wrote:
> Use logging.debug()

Done.

http://codereview.chromium.org/1640001/diff/1/2#newcode836
gclient.py:836: thread_pool.Start()
On 2010/04/10 01:16:58, Marc-Antoine Ruel wrote:
> I'd prefer:
> 
> thread_pool.Start()
> try:
>   ...
> finally:
>   thread_pool.WaitJobs()

I did a big try: ... finally: thread_pool.Stop().
Stop() does a WaitJobs anyway, and I think what we're really looking for is to
make sure all the threads are joined. I tested a bunch of conditions (jobs
failing, hitting ctrl-C), and with that and another change in the thread_pool
class, it was consistently stopping correctly

http://codereview.chromium.org/1640001/diff/1/3
File tests/gclient_test.py (right):

http://codereview.chromium.org/1640001/diff/1/3#newcode304
tests/gclient_test.py:304: for key in dir(options):
On 2010/04/10 01:16:58, Marc-Antoine Ruel wrote:
> There's an error in your code. Here's a better version:
> 
> a = [i for i in dir(options) if not i.startswith('_')]
> b = [i for i in dir(others) if not i.startswith('_')]
> if a != b:
>   return False
> try:
>   for key in a:
>     if getattr(a, key) != getattr(b, key):
>       return False
>   return True
> except AttributeError:
>   return False
> 
> You can rename a and b :)

Done, and checked tests again.

[M-A Ruel, 2010-04-13 01:15:09]

On 2010/04/12 23:51:57, piman wrote:
> PTAL. If you'd rather wait on a week-end for the checkin, let me know. We
don't
> have any canary system for gclient, do we ?

No, we had one years ago but it wasn't used so it was basically useless.

> I tested with wiping the svn credentials. Surprisingly, I was able to log on
the
> command line when using -j10, even though the svn messages are somewhat
> confusing. It works with no -j option as well, expectedly.

Perfect.


> http://codereview.chromium.org/1640001/diff/1/2#newcode836
> gclient.py:836: thread_pool.Start()
> On 2010/04/10 01:16:58, Marc-Antoine Ruel wrote:
> > I'd prefer:
> > 
> > thread_pool.Start()
> > try:
> >   ...
> > finally:
> >   thread_pool.WaitJobs()
> 
> I did a big try: ... finally: thread_pool.Stop().
> Stop() does a WaitJobs anyway, and I think what we're really looking for is to
> make sure all the threads are joined. I tested a bunch of conditions (jobs
> failing, hitting ctrl-C), and with that and another change in the thread_pool
> class, it was consistently stopping correctly

Good.

lgtm

[M-A Ruel, 2010-04-13 01:15:20]

http://codereview.chromium.org/1640001/diff/10001/11001
File gclient.py (right):

http://codereview.chromium.org/1640001/diff/10001/11001#newcode800
gclient.py:800: # Instead of duplicating the tests, it's less fragile to just
ignore the
80 cols :(

[piman, 2010-04-13 01:40:56]

http://codereview.chromium.org/1640001/diff/10001/11001
File gclient.py (right):

http://codereview.chromium.org/1640001/diff/10001/11001#newcode800
gclient.py:800: # Instead of duplicating the tests, it's less fragile to just
ignore the
On 2010/04/13 01:15:20, Marc-Antoine Ruel wrote:
> 80 cols :(

Fixed (here and below).

[piman, 2010-04-17 01:17:38]

On Mon, Apr 12, 2010 at 6:40 PM, <piman@chromium.org> wrote:

>
> http://codereview.chromium.org/1640001/diff/10001/11001
> File gclient.py (right):
>
> http://codereview.chromium.org/1640001/diff/10001/11001#newcode800
> gclient.py:800: # Instead of duplicating the tests, it's less fragile to
> just ignore the
> On 2010/04/13 01:15:20, Marc-Antoine Ruel wrote:
>
>> 80 cols :(
>>
>
> Fixed (here and below).
>
>
> http://codereview.chromium.org/1640001/show
>

I did some extra testing, and so did another person, didn't find any issue.
When do you think it is a good time to check in ?

Antoine

[M-A Ruel, 2010-04-17 01:42:50]

On 2010/04/17 01:17:38, piman wrote:
> I did some extra testing, and so did another person, didn't find any issue.
> When do you think it is a good time to check in ?

As soon as you want. lgtm.

[piman, 2010-04-17 01:54:38]

On Fri, Apr 16, 2010 at 6:42 PM, <maruel@chromium.org> wrote:

> On 2010/04/17 01:17:38, piman wrote:
>
>> I did some extra testing, and so did another person, didn't find any
>> issue.
>> When do you think it is a good time to check in ?
>>
>
> As soon as you want. lgtm.


It's in, crossing fingers....


>
>
> http://codereview.chromium.org/1640001/show
>

[piman, 2010-04-17 02:55:07]

On Fri, Apr 16, 2010 at 6:54 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
> On Fri, Apr 16, 2010 at 6:42 PM, <maruel@chromium.org> wrote:
>
>> On 2010/04/17 01:17:38, piman wrote:
>>
>>> I did some extra testing, and so did another person, didn't find any
>>> issue.
>>> When do you think it is a good time to check in ?
>>>
>>
>> As soon as you want. lgtm.
>
>
> It's in, crossing fingers....
>

It broke the WebKit bots because of python2.5isms.
I'll have to start over...

Antoine


>
>
>>
>>
>> http://codereview.chromium.org/1640001/show
>>
>
>

[piman, 2010-04-20 23:22:40]

PTAL. I fixed it to run with python 2.4:
- fixed finally clause syntax
- re-implemented inexistent API in Queue

[M-A Ruel, 2010-04-21 00:56:42]

lgtm

[Nasser Grainawi, 2010-04-21 15:57:45]

This seems to have some unwanted behavior when you try to Ctrl+C with -j1. It
proceeds to go onto the next item in the deps and try to sync it, and then the
next one, and eventually I got stuck in some python loop and had to kill the
python process from another shell. :(

[M-A Ruel, 2010-04-21 16:55:32]

On 2010/04/21 15:57:45, Nasser Grainawi wrote:
> This seems to have some unwanted behavior when you try to Ctrl+C with -j1. It
> proceeds to go onto the next item in the deps and try to sync it, and then the
> next one, and eventually I got stuck in some python loop and had to kill the
> python process from another shell. :(

Would the best fix be to handle all exceptions in worker threads and raise them
back in the primary thread?

[Nasser Grainawi, 2010-04-21 17:26:29]

maruel@chromium.org wrote:
> On 2010/04/21 15:57:45, Nasser Grainawi wrote:
>> This seems to have some unwanted behavior when you try to Ctrl+C with 
>> -j1. It
>> proceeds to go onto the next item in the deps and try to sync it, and 
>> then the
>> next one, and eventually I got stuck in some python loop and had to 
>> kill the
>> python process from another shell. :(
> 
> Would the best fix be to handle all exceptions in worker threads and 
> raise them
> back in the primary thread?

That should work.

> 
> http://codereview.chromium.org/1640001/show

[piman, 2010-04-21 18:43:25]

On Wed, Apr 21, 2010 at 10:26 AM, Nasser Grainawi <nasser@codeaurora.org>wrote:

> maruel@chromium.org wrote:
>
>> On 2010/04/21 15:57:45, Nasser Grainawi wrote:
>>
>>> This seems to have some unwanted behavior when you try to Ctrl+C with
>>> -j1. It
>>> proceeds to go onto the next item in the deps and try to sync it, and
>>> then the
>>> next one, and eventually I got stuck in some python loop and had to kill
>>> the
>>> python process from another shell. :(
>>>
>>
>> Would the best fix be to handle all exceptions in worker threads and raise
>> them
>> back in the primary thread?
>>
>
> That should work.


That's already what it's supposed to be doing...

I'l investigate more.

Antoine

>
>
>
>> http://codereview.chromium.org/1640001/show
>>
>
>
>

[piman, 2010-04-22 03:57:31]

With this new version, ctrl-c works on python 2.5+, but I can't figure out how
to make ctrl-c work on python 2.4.
How much do you think we should care, given that python 2.4 is added for the
webkit buildbots, which are not expected to receive interactive input ?

[M-A Ruel, 2010-04-22 12:50:10]

I don't think it's a big deal, 2.4 is slowly being phased so as long as
non-interactive execution isn't broken it should be fine. I can't review
right now,  will take a look in a few hours, ping me if I'm too slow.

Le 21 avr. 2010 23:57, <piman@chromium.org> a écrit :
> With this new version, ctrl-c works on python 2.5+, but I can't figure out

> how
> to make ctrl-c work on python 2.4.
> How much do you think we should care, given that python 2.4 is added for
the
> webkit buildbots, which are not expected to receive interactive input ?
>
> http://codereview.chromium.org/1640001/show

[M-A Ruel, 2010-04-22 16:01:47]

lgtm

[jamesr, 2010-04-27 01:51:07]

I think you broke the build.webkit.org chromium builders, they are all dying
with an error similar to this:

Traceback (most recent call last):
  File "c:\depot_tools\bootstrap\\..\gclient.py", line 1380, in ?
    result = Main(sys.argv)
  File "c:\depot_tools\bootstrap\\..\gclient.py", line 1375, in Main
    return DispatchCommand(command, options, args)
  File "c:\depot_tools\bootstrap\\..\gclient.py", line 1284, in DispatchCommand
    return command_map[command](options, args)
  File "c:\depot_tools\bootstrap\\..\gclient.py", line 1201, in DoUpdate
    return client.RunOnDeps('update', args)
  File "c:\depot_tools\bootstrap\\..\gclient.py", line 850, in RunOnDeps
    content =  gclient_utils.FileRead(filename)
  File "c:\depot_tools\gclient_utils.py", line 118, in FileRead
    f = open(filename, mode)
IOError: [Errno 2] No such file or directory:
'C:\\WebKitBuildSlave\\chromium-win-release\\build\\WebKit\\chromium\\chromium_deps\\DEPS'
No such file or directory at ./WebKitTools/Scripts/update-webkit-chromium line
55.
program finished with exit code 2

Tony Chang is working on a revert.

[piman, 2010-04-27 03:01:39]

PTAL... Hopefully that'll work this time. I ran update-webkit-chromium and that
seems to work, but I'm not sure what it's supposed to do exactly.

[piman, 2010-04-27 03:08:19]

On Mon, Apr 26, 2010 at 8:01 PM, <piman@chromium.org> wrote:

> PTAL... Hopefully that'll work this time. I ran update-webkit-chromium and
> that
> seems to work, but I'm not sure what it's supposed to do exactly.


I also updated the test so that it would catch the error.


>
>
> http://codereview.chromium.org/1640001/show
>

[tony, 2010-04-27 03:21:37]

The changes from patchset 7 LGTM.

[M-A Ruel, 2010-04-27 13:31:42]

I don't fully grasp the subtlety in this area but it lgtm.

Desperately need a smoke test...

[piman, 2010-06-18 05:19:52]

Ok, so after a long hiatus, this CL is back. I rebased to top-of-trunk.
I added a couple of fixes:
- ctrl-c works, for real. the interaction between threading and signals in
python is broken - the bottom line is that you need to have your locks in the
main thread to have a timeout if you want signals to come through.
- I changed the default to be 0 threads, and that makes the thread pool execute
the jobs right as it receives them. It makes the default much closer to the
current behavior.
- I fixed the progress meter (except it now updates at the end of the job rather
than the beginning, it's a somewhat more accurate measure when you have many
threads).

This passes the smoke tests.

[M-A Ruel, 2010-06-18 13:58:38]

I'm sorry to throttle you down but do you mind waiting for
http://codereview.chromium.org/2627007.

If you look at this review, you'll see each dependency in each DEPS file is a
Dependency object on its own, making dependency management much saner than with
a flat dict object. I've been splitting up this review in little bits for almost
two weeks now, the last change to be reviewed being
http://codereview.chromium.org/2839008/show.

Note that I really want parallel checkout to be enabled ASAP but the refactor
I've been doing conflicts a lot with this change. At least you won't need a
closure anymore. Each Dependency() can have its own subprocess.Popen() handle
and multiplex the output in a single thread, no need for multiple threads
anymore either.

[piman, 2010-06-18 21:14:10]

On Fri, Jun 18, 2010 at 6:58 AM, <maruel@chromium.org> wrote:

> I'm sorry to throttle you down but do you mind waiting for
> http://codereview.chromium.org/2627007.
>
> If you look at this review, you'll see each dependency in each DEPS file is
> a
> Dependency object on its own, making dependency management much saner than
> with
> a flat dict object. I've been splitting up this review in little bits for
> almost
> two weeks now, the last change to be reviewed being
> http://codereview.chromium.org/2839008/show.
>
> Note that I really want parallel checkout to be enabled ASAP but the
> refactor
> I've been doing conflicts a lot with this change. At least you won't need a
> closure anymore. Each Dependency() can have its own subprocess.Popen()
> handle
> and multiplex the output in a single thread, no need for multiple threads
> anymore either.


I'll wait and rewrite it then.
I'd like to get rid of threads if possible. I won't be able to get rid of
the closures, because some work happens after the subprocess spawns that
depend on their result, but closures aren't a bad thing. It may also fix the
ordering (which wasn't an issue in my experience, but I'd feel more
comfortable if fixed).


>
>
> http://codereview.chromium.org/1640001/show
>