upload_dir_contents(): upload multiple files in parallel

upload_dir_contents(): upload multiple files in parallel

BUG=skia:2780
R=borenet@google.com

Committed: https://skia.googlesource.com/common/+/0b62f0e

[epoger, 2014-07-27 16:37:08]

Eric- PTAL as of patchset 2.

[borenet, 2014-07-28 13:32:11]

Looks mostly good, but I don't like the end_of_queue object...

https://codereview.chromium.org/424553002/diff/20001/py/utils/gs_utils.py
File py/utils/gs_utils.py (right):

https://codereview.chromium.org/424553002/diff/20001/py/utils/gs_utils.py#new...
py/utils/gs_utils.py:366: num_threads = num_files_to_upload
Maybe line 358 should be moved here so that it doesn't have to run when
num_files_to_upload == 0?

https://codereview.chromium.org/424553002/diff/20001/py/utils/gs_utils.py#new...
py/utils/gs_utils.py:374: return
Why not use q.get(False) or q.get_nowait() and try/except Queue.Empty?  That
avoids having to use a marker object.

[epoger, 2014-07-28 14:14:14]

Eric- Thanks (as always!) for your fast and thorough review.  Comments inline.

https://codereview.chromium.org/424553002/diff/20001/py/utils/gs_utils.py
File py/utils/gs_utils.py (right):

https://codereview.chromium.org/424553002/diff/20001/py/utils/gs_utils.py#new...
py/utils/gs_utils.py:366: num_threads = num_files_to_upload
On 2014/07/28 13:32:11, borenet wrote:
> Maybe line 358 should be moved here so that it doesn't have to run when
> num_files_to_upload == 0?

Done.  I don't think there is significant performance impact either way, because
all that line does is set up a "pointer" to the iter().next function call.  I'm
happy with the code either way.

https://codereview.chromium.org/424553002/diff/20001/py/utils/gs_utils.py#new...
py/utils/gs_utils.py:374: return
On 2014/07/28 13:32:11, borenet wrote:
> Why not use q.get(False) or q.get_nowait() and try/except Queue.Empty?  That
> avoids having to use a marker object.

The problem with checking for an empty queue is that we don't know WHY the queue
is empty: maybe the main thread finished writing tasks into it, or maybe it
hasn't had a chance to start writing tasks into it yet?  Or maybe it is in the
middle of writing tasks into the queue, and paused for a moment?

AFAIK the only ways to handle this without an end marker are:
1. Allow the worker threads to live forever, blocking on the queue, waiting for
tasks that never come.  Seems wasteful.
2. Make the main thread write all of its tasks into the queue before spinning up
the worker threads.  In this particular case that would probably be fine, but in
general it's nice to be able to start the workers while the main thread is still
filling the task queue.

Would you prefer one of those approaches here?  Or maybe there's another option
I haven't thought of?

[borenet, 2014-07-28 14:20:07]

https://codereview.chromium.org/424553002/diff/20001/py/utils/gs_utils.py
File py/utils/gs_utils.py (right):

https://codereview.chromium.org/424553002/diff/20001/py/utils/gs_utils.py#new...
py/utils/gs_utils.py:374: return
On 2014/07/28 14:14:14, epoger wrote:
> On 2014/07/28 13:32:11, borenet wrote:
> > Why not use q.get(False) or q.get_nowait() and try/except Queue.Empty?  That
> > avoids having to use a marker object.
> 
> The problem with checking for an empty queue is that we don't know WHY the
queue
> is empty: maybe the main thread finished writing tasks into it, or maybe it
> hasn't had a chance to start writing tasks into it yet?  Or maybe it is in the
> middle of writing tasks into the queue, and paused for a moment?
> 
> AFAIK the only ways to handle this without an end marker are:
> 1. Allow the worker threads to live forever, blocking on the queue, waiting
for
> tasks that never come.  Seems wasteful.
> 2. Make the main thread write all of its tasks into the queue before spinning
up
> the worker threads.  In this particular case that would probably be fine, but
in
> general it's nice to be able to start the workers while the main thread is
still
> filling the task queue.
> 
> Would you prefer one of those approaches here?  Or maybe there's another
option
> I haven't thought of?

I was thinking of #2, since you're dealing with the special case of never
needing to add things to the queue after the initial set.  My unfounded
intuition is that the performance difference will be minimal, and the code gets
a little simpler.  Really, I'm fine either way.

[epoger, 2014-07-28 14:36:55]

On 2014/07/28 14:20:07, borenet wrote:
> I was thinking of #2, since you're dealing with the special case of never
> needing to add things to the queue after the initial set.  My unfounded
> intuition is that the performance difference will be minimal, and the code
gets
> a little simpler.  Really, I'm fine either way.

Makes sense!  Please see patchset 4.

As an added bonus of filling the queue at the beginning, I was able to get rid
of atomic_incrementer.

[borenet, 2014-07-28 14:41:33]

LGTM with optional nit.

https://codereview.chromium.org/424553002/diff/60001/py/utils/gs_utils.py
File py/utils/gs_utils.py (right):

https://codereview.chromium.org/424553002/diff/60001/py/utils/gs_utils.py#new...
py/utils/gs_utils.py:290: num_threads=10, upload_if=UploadIf.ALWAYS, **kwargs):
Optional nit: Would slightly prefer that this be a constant, or maybe even
intelligently use a function of the number of cores on the machine.

[epoger, 2014-07-28 14:50:05]

https://codereview.chromium.org/424553002/diff/60001/py/utils/gs_utils.py
File py/utils/gs_utils.py (right):

https://codereview.chromium.org/424553002/diff/60001/py/utils/gs_utils.py#new...
py/utils/gs_utils.py:290: num_threads=10, upload_if=UploadIf.ALWAYS, **kwargs):
On 2014/07/28 14:41:33, borenet wrote:
> Optional nit: Would slightly prefer that this be a constant, or maybe even
> intelligently use a function of the number of cores on the machine.

Done.

[epoger, 2014-07-28 14:50:24]

Committed patchset #5 manually as r0b62f0e (presubmit successful).