Add --jobs support to gclient. --jobs=1 is still the default for now.

Add --jobs support to gclient. --jobs=1 is still the default for now.

Huge thanks to piman@ for working on a patch. I chose a different design but he
gave me motivation and ideas. Sorry for not accepting his patch earlier, this
was mostly due to broken gclient implementation itself.

gclient can now run an unlimited number of parallel checkouts and always keep
the checkout coherency correct.

--jobs=1 is single threaded as before, albeit with a different code path.

Issues:
- Using --jobs with a value other than 1 will result in a mangled output.
- Exceptions thrown in a thread will be have the wrong stack trace.

TEST=gclient sync -j 99 in a ssh:// chromiumos checkout is dramatically faster.

---

Here's the perf on linux on i7-860 for a chromium checkout with warm cache. Cold
cache will result is significantly reduced improvements so this is best case
improvements. The sync was no-op all the time except where noted. All execution
where with "time gclient sync " + args. Didn't include 'sys' column since it was
statistically insignifiant and highly correlated with 'user'.

           runs with -f        runs with -m      without -f nor -m
 args          real    user      real    user      real    user
 -j 12       20.59s  18.00s     5.64s   7.95s     5.86s   8.10s
        #1 1m05.26s  20.02s     5.20s   7.94s     5.10s   8.09s
             22.79s  18.17s
 -j 1   #2 1m47.00s  16.72s     9.69s   5.72s    12.35s   5.96s
           1m31.28s  17.06s     9.54s   5.85s    10.51s   6.20s
           1m31.79s  16.39s
 before #3 1m30.94s  16.74s     9.77s   5.83s    10.45s   5.77s
           1m30.17s  17.30s    10.36s   5.68s    10.16s   5.88s
 hook #4      8.52s  7.93s
              8.73s  8.13s

#1 This particular run synched to r56023, a webkit roll updating layout tests.
   It's still faster than a no-op sync without parallel checkout.
#2 Maybe there was a sync or computer hickup, I didn't realize.
#3 This is depot_tools@56020
#4 Since -f implies runhooks, I ran the hook 'python src/build/gyp_chromium'
   manually to compare. Hooks are still run in a single thread. I didn't rest
   'gclient runhooks'.

I tried to go a ssh:// checkout of chromium os tree but it timed out everytime I
tried to sync so I couldn't get data points. I expect an order of magnitude of
improvement or more.

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

[M-A Ruel, 2010-08-13 16:41:52]

I kept the default to 1 the time I fine tune the default a bit.

Contrary to http://codereview.chromium.org/1640001, this patch avoids keeping
any thread knowledge in gclient.py. I think it keeps the Dependency code easier
to follow. It also doesn't keep a closure but just packages *args and **kwargs.

There is one bug lurking because some code in gclient.py mutates options. I need
to fix that before enabling jobs > 1.

[M-A Ruel, 2010-08-13 16:54:08]

Fixed the description (lines starting with # were ignored) and fixed the
mutation bug while at it.

[M-A Ruel, 2010-08-13 16:56:18]

And for the record:

           runs with -f        runs with -m      without -f nor -m
 args          real    user      real    user      real    user
 -j 12       20.59s  18.00s     5.64s   7.95s     5.86s   8.10s
        #1 1m05.26s  20.02s     5.20s   7.94s     5.10s   8.09s
             22.79s  18.17s
 -j 1   #2 1m47.00s  16.72s     9.69s   5.72s    12.35s   5.96s
           1m31.28s  17.06s     9.54s   5.85s    10.51s   6.20s
           1m31.79s  16.39s
 before #3 1m30.94s  16.74s     9.77s   5.83s    10.45s   5.77s
           1m30.17s  17.30s    10.36s   5.68s    10.16s   5.88s
 hook #4      8.52s  7.93s
              8.73s  8.13s


Le 13 août 2010 12:54, <maruel@chromium.org> a écrit :

> Fixed the description (lines starting with # were ignored) and fixed the
> mutation bug while at it.
>
>
> http://codereview.chromium.org/3135014/show
>

[bradn, 2010-08-13 17:23:25]

Generally looks ok.
I'd have spawned a fixed number of workers and had then drain the work pool, but
this works too.
Haven't looked, but is there one place where subprocess get called to run the
commands? Could we string an extra arg for a 'print' mutex down RunCommand() and
then use PIPE/communicate and have each sync sit on their output until
completion? I guess you'd probably want a timeout too.
That can wait if you want to do it incrementally though.

http://codereview.chromium.org/3135014/diff/3001/4002
File gclient_utils.py (right):

http://codereview.chromium.org/3135014/diff/3001/4002#newcode394
gclient_utils.py:394: self.cond = threading.Condition()
Maybe a more descriptive name for the condition, like ready_to_run?

http://codereview.chromium.org/3135014/diff/3001/4002#newcode397
gclient_utils.py:397: self.wake_up = True
ready_to_run and ready_to_run_cond above?

http://codereview.chromium.org/3135014/diff/3001/4002#newcode499
gclient_utils.py:499: # finish before this worker thread. Since this thread is a
daemon,
That sounds kinda gross.
Could you maybe accumulate a list of threads as you spawn then so you can wait
on them at the end up above?

[M-A Ruel, 2010-08-13 19:21:13]

On 2010/08/13 17:23:25, bradn wrote:
> Generally looks ok.
> I'd have spawned a fixed number of workers and had then drain the work pool,
but
> this works too.
> Haven't looked, but is there one place where subprocess get called to run the
> commands? Could we string an extra arg for a 'print' mutex down RunCommand()
and
> then use PIPE/communicate and have each sync sit on their output until
> completion? I guess you'd probably want a timeout too.
> That can wait if you want to do it incrementally though.

Using a thread pool would be in theory cleaner but in practice it doesn't
matter, it's a micro optimization and will simply result in more code. I wanted
the implementation as compact as possible.

w.r.t. pipies, it's fairly involved so it's out of scope for this change, it'll
be done later since it involves changing much more code.

http://codereview.chromium.org/3135014/diff/3001/4002
File gclient_utils.py (right):

http://codereview.chromium.org/3135014/diff/3001/4002#newcode397
gclient_utils.py:397: self.wake_up = True
On 2010/08/13 17:23:25, bradn wrote:
> ready_to_run and ready_to_run_cond above?

wake_up was a stale variable, I removed it.

---

I heartedly hate condition variables since it's both a lock and an auto reset
event. I'd rather have an auto reset event and manage locking myself but
threading.Event is not an auto-reseting, so it's basically useless.

python is really not multithreading-friendly in all possible ways.

http://codereview.chromium.org/3135014/diff/3001/4002#newcode499
gclient_utils.py:499: # finish before this worker thread. Since this thread is a
daemon,
On 2010/08/13 17:23:25, bradn wrote:
> That sounds kinda gross.
> Could you maybe accumulate a list of threads as you spawn then so you can wait
> on them at the end up above?

That was gross but not really important, the comment made it look worse that it
was. Anyway I changed the behavior.

http://codereview.chromium.org/3135014/diff/9001/10002#newcode439
gclient_utils.py:439: break
It's inefficient to regenerate the array all the time but using
self.running.remove() will probably be even slower.

[piman, 2010-08-13 19:26:06]

http://codereview.chromium.org/3135014/diff/3001/4002
File gclient_utils.py (right):

http://codereview.chromium.org/3135014/diff/3001/4002#newcode462
gclient_utils.py:462: self.cond.wait()
Does ctrl-c work ?
In my previous CL I had to work-around threading issues in python:
- signals are only delivered to the main thread
- synchronization primitives and join hold the GIL while waiting and so they
don't get the KeyboardInterrupt.

Work around is simply to add a timeout (and loop if needed, in this case I don't
think it is).

See http://codereview.chromium.org/1640001/diff2/45001:52001/53002

[bradn, 2010-08-13 19:29:14]

I'm not completely sure the thread pool would be a micro optimization. In
particular, would allow you to bound the number of simultaneous syncs, its
possible that above some number, you're performance would degrade as the disk
would be syncing like mad. Currently it basically spawns as much as possible.
But anyways, premature optimization....
LGTM

http://codereview.chromium.org/3135014/diff/3001/4002
File gclient_utils.py (right):

http://codereview.chromium.org/3135014/diff/3001/4002#newcode397
gclient_utils.py:397: self.wake_up = True
On 2010/08/13 19:21:13, Marc-Antoine Ruel wrote:
> On 2010/08/13 17:23:25, bradn wrote:
> > ready_to_run and ready_to_run_cond above?
> 
> wake_up was a stale variable, I removed it.
> 
> ---
> 
> I heartedly hate condition variables since it's both a lock and an auto reset
> event. I'd rather have an auto reset event and manage locking myself but
> threading.Event is not an auto-reseting, so it's basically useless.
> 
> python is really not multithreading-friendly in all possible ways.

I think you can create a Condition that's tethered to a separate mutex (pass the
mutex into the condition construtor). Mixing the two does seem weird. Might make
it more readable.

[piman, 2010-08-13 19:31:35]

On Fri, Aug 13, 2010 at 12:29 PM, <bradnelson@google.com> wrote:

> I'm not completely sure the thread pool would be a micro optimization. In
> particular, would allow you to bound the number of simultaneous syncs, its
> possible that above some number, you're performance would degrade as the
> disk
> would be syncing like mad. Currently it basically spawns as much as
> possible.
>

I thought it did that too, and was going to suggest the same, but it
doesn't: it only spawns a thread if len(running) < self.jobs.


> But anyways, premature optimization....
> LGTM
>
>
>
> http://codereview.chromium.org/3135014/diff/3001/4002
> File gclient_utils.py (right):
>
> http://codereview.chromium.org/3135014/diff/3001/4002#newcode397
> gclient_utils.py:397: self.wake_up = True
> On 2010/08/13 19:21:13, Marc-Antoine Ruel wrote:
>
>> On 2010/08/13 17:23:25, bradn wrote:
>> > ready_to_run and ready_to_run_cond above?
>>
>
>  wake_up was a stale variable, I removed it.
>>
>
>  ---
>>
>
>  I heartedly hate condition variables since it's both a lock and an
>>
> auto reset
>
>> event. I'd rather have an auto reset event and manage locking myself
>>
> but
>
>> threading.Event is not an auto-reseting, so it's basically useless.
>>
>
>  python is really not multithreading-friendly in all possible ways.
>>
>
> I think you can create a Condition that's tethered to a separate mutex
> (pass the mutex into the condition construtor). Mixing the two does seem
> weird. Might make it more readable.
>
>
> http://codereview.chromium.org/3135014/show
>

[bradn, 2010-08-13 19:36:00]

Ah yes, I see, it breaks and the waits, cool, ok.

[M-A Ruel, 2010-08-13 19:55:57]

Add polling so Ctrl-C still work. (Sadness) Note that I set it to 10 seconds
since it just works immediately as long as there is a timeout value.

[piman, 2010-08-13 20:12:10]

http://codereview.chromium.org/3135014/diff/9001/10002
File gclient_utils.py (right):

http://codereview.chromium.org/3135014/diff/9001/10002#newcode486
gclient_utils.py:486: self.daemon = True
I think you want to keep the threads as daemons. Otherwise, if the main thread
dies, ctrl-c can't kill the threads (it will cause the supbrocess to quit, but
not the thread itself).

[M-A Ruel, 2010-08-13 20:23:20]

http://codereview.chromium.org/3135014/diff/9001/10002
File gclient_utils.py (right):

http://codereview.chromium.org/3135014/diff/9001/10002#newcode486
gclient_utils.py:486: self.daemon = True
On 2010/08/13 20:12:10, piman wrote:
> I think you want to keep the threads as daemons. Otherwise, if the main thread
> dies, ctrl-c can't kill the threads (it will cause the supbrocess to quit, but
> not the thread itself).

Since the threads never wait while holding the lock, they will quit in a timely
manner all the time so I'd prefer to not make them daemon. In my limited testing
it wasn't an issue at least.

[piman, 2010-08-13 20:31:32]

On Fri, Aug 13, 2010 at 1:23 PM, <maruel@chromium.org> wrote:

>
> http://codereview.chromium.org/3135014/diff/9001/10002
> File gclient_utils.py (right):
>
> http://codereview.chromium.org/3135014/diff/9001/10002#newcode486
> gclient_utils.py:486: self.daemon = True
> On 2010/08/13 20:12:10, piman wrote:
>
>> I think you want to keep the threads as daemons. Otherwise, if the
>>
> main thread
>
>> dies, ctrl-c can't kill the threads (it will cause the supbrocess to
>>
> quit, but
>
>> not the thread itself).
>>
>
> Since the threads never wait while holding the lock, they will quit in a
> timely manner all the time so I'd prefer to not make them daemon. In my
> limited testing it wasn't an issue at least.


Fair enough, LGTM.

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