[WIP] Parallelised gps.WriteDepTree

[aneshas]

Fixes #895

gps.WriteDepTree now writes vendor/ tree concurrently with
configurable number of worker routines.

Added BenchmarkCreateVendorTreeParallel benchmark which
runs 1/3 faster than when running sequentially.

Your feedback is welcome.
I added NumDepTreeWorkers as a global at this point will change it based on the feedback.

[sdboyer]

good start 😄

[sdboyer]

Let's add this as a parameter to WriteDepTree(). We've avoided package global state in gps so far, and want to continue that trend 😄

[aneshas]

np, I was unsure if you wanted to change the func signature :D

[sdboyer]

yep! we haven't stabilized the API yet, so there's no obligation to keep it the same.

[sdboyer]

let's create some local tuple type here for this message - something like

type done struct {
	root ProjectRoot
	err error
}

so that we don't have to shoehorn all that information into the error, and we can pass back up the lower-level errors unmodified.

[aneshas]

ok, no problem, I didn't want to add too much complexity, didn't know to which extent you want this to be modified. I get your point, will do :)

[sdboyer]

for sure, that's how we figure these things out 😄

[sdboyer]

so, my default position is that there are only two correct values for channel sizes: 0, and 1. while we could do it this way, it's creating problems elsewhere (like early termination, noted below). i think a refactored implementation that uses unbuffered channels would give us tighter control over such things.

[sdboyer]

as-written, this could interleave poorly:

1. one goroutine sends experiences and sends an error while other(s) still writing
2. control receives the error, and begins os.RemoveAll()
3. other goroutine(s) are trying to write to dirs that are disappearing from underneath them

additionally, it's also crucial that we guarantee the worker goroutines have terminated by the time the WriteDepTree() func returns.

switching to an unbuffered channel approach will, i think, make it easier to do the necessary flow control here.

[aneshas]

so, what you are saying is basically this:

we should always wait for all worker routines to finish
if some routine reports an error, note the error and wait for others to finish
return a fmt.Errorf filled with info of all errs (gathered from the tuples received - if any)

[aneshas]

I will refactor and make another pr tomorrow and we can discuss further :)

[aneshas]

@sdboyer While doing the refactor I kinda got the idea that WriteDepTree itself was
getting to clunky and it will get eve larger so I wanted to brake it down to child functions but as I went down even the signature is kinda strange it already has 4 parameters adding workerCount pumps
it up to 5 ...

If you can see what I wanted to do with depTreeJob it kinda ocuured to me that instead of receiving
5 arguments WriteDepTree could receive the depTreeJob itself (whatever the name will be) as config ??

Or should the WriteDepTree be a method on let's say DepTree (which would be instantiated via constructor) .. ?
eg.

tree := gps.NewDepTree(gps.DepTreeConfig{
    Basedir: "...",
    Lock: ...,
    SourceManager: ...,
})

err := tree.Write(true, 2) // stripVendor, workerCount
...

Or do you just want to leave it as it is, but it would be ugly?

I hope you can see what I'm getting at

[sdboyer]

yes, i see the general idea of what you're going for.

as long as we're talking about the signature design, it's worth noting that we'll also need to consider that the sv param will eventually become a bitmask, and that we'll need to add a context.Context param for cancellation control.

we have a few options, not all mutually exclusive:

1. "hide" one parameter by making this a method, presumably of SourceManager
2. condense basedir, parallelism and stripping controls into a config struct
3. don't allow external control over parallelism - this func is basically a convenience wrapper, relying on no particularly magical internal logic (except the stripVendor func, but whatever), so dependers are freely able to reimplement it
4. just eat the larger number of params

i'm actually inclined to stick with 3+4, because i don't see us adding much more in the way of params to this in the future, and we have to balance the convenience of this one method with cluttering the overall gps namespace with helpers.

[aneshas]

I see.
So let's say we stick with 3+4. By saying "don't allow external control over parallelism" what do you suggest we do with the workerCount? Make it fixed or.. ?
And even if it is just a convenience it would still probably be good to break it down to a few smaller funcs, or you suggest that I don't do that also?

[sdboyer]

(sorry, I realize this is pulling a 180 on the concurrency thing)

probably start with just plain fixed, and then maybe we make some adjustments based on GOMAXPROCS.

if you think that splitting it up into some smaller funcs will help, then sure, go for it. as long as they're not exported, there's no general rules design rules in place - we can discuss the design as you go.

[aneshas]

@sdboyer when you have time could you look at the latest update

[sdboyer]

sorry for delayed response! looks like we're heading in a better direction, but we're still missing semantics for aborting early in the event of an error on any individual attempt.

note that whatever approach you take should ideally also be easy to extend with a context.Context() for explicit cancellation semantics, later.

[sdboyer]

sorry, this ended up being finished up in another PR 😢