Feature/concurrent dcopy #123
Conversation
* Use worker pool * Introduce Concurrency() functional option * Collect all errors * Simplify type of Concurrency option * Make channels buffered * Make input channel unbuffered * Logging for debugging * 10x channel capacities * 10x concurrency * 100x concurrency * 100x in channel * Make output channel 1000x * Reducing numWorkers multiplier by 1/10 * Removing numWorkers multiplier * Reducing in channel capacity by 1/10 * Reducing output channel capacity by 1/10 * Reducing output channel capacity by further 1/10 * Remove input channel capacity multiplier * Removing multiplication factor for output channel capacity * Remove debugging statement --------- Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com>
This reverts commit a403ffc.
|
@ycombinator Please take a look and comment. Please try this version on your local as well if possible. |
|
@ycombinator any comment on this? |
|
@otiai10 I have not yet had time to look at this. My plan is to look at it tomorrow (Friday). I took a quick glance at this PR, though, and noticed one major difference from my implementation in #120 is that in this one you are only concurrently copying directories. Whereas, in 120, all files were being copied concurrently. Any reason for this difference? |
|
When we call
Thus I did like this. But I found a critical bug of this implementation in another context, thus I'll close this. |
Since it acquires semaphore worker for directory and locks until it ends, there might be "never-ending" semaphore.
errorgroup.WithContext can cancel sibling routines when a routine returns non-nil error. See https://cs.opensource.google/go/x/sync/+/refs/tags/v0.3.0:errgroup/errgroup.go;l=40-48
|
Fixed |
There was a problem hiding this comment.
LGTM - thank you!
I tested this PR with elastic/elastic-agent#3212 (comment) and updated the performance results in the table. There is definitely a speed improvement!
|
@shiwork That would be great if you can leave some comment on this PR 🙇 |
|
Hi @otiai10, any plans to merge this PR? We are trying to use this improvement in Elastic Agent during installation (see elastic/elastic-agent#3212). Thanks! |
|
@ycombinator Thank you for pinging. I'll get back to you soon 🙇 |
|
@ycombinator Released under v1.14.0 |
Fix #81
Fix #120