make pushes concurrent and limit pull concurrency

[dicej]

Previously, pushes were serialized and pulls were unboundedly concurrent. This makes pushes concurrent (capped at 16 concurrent uploads) and also caps download concurrency at 16.

[bacongobbler]

Thanks @dicej!

Do you think it'd be useful if the maximum number of concurrent pulls/pushes was dependent on the system hardware? For example, a Raspberry Pi may want to serialize less concurrent pulls/pushes, while a more beefy dev workstation may want to increase that count.

Most programs make this configurable by reading the number of available CPUs and provide an option to modify the default in case there are specific use cases (throttling/benchmarking).

What do you think about that approach?

[dicej]

@bacongobbler Given that uploads and downloads are generally I/O limited rather than CPU-limited, I don't think the number of CPU cores is an appropriate metric to use. For example, a Raspberry Pi on a gigabit internet connection is likely to perform better than a 12 core laptop tethered to a slow cellphone connection. I do agree that adjusting the number based on some kind of runtime detection would be great; just not sure what that detection would look like (or whether it would be worthwhile).

[bacongobbler]

Hmm. Perhaps it could be an optional field in the ClientConfig, defaulting to MAX_PARALLEL_PUSH_AND_PULL if unset. In the future we could be smarter about the default, but that would allow clients to change the default (or revert back to serialized pull/push if that was required). Would you agree with that approach for now?

[bacongobbler]

I wonder if we need to make two separate options: one for uploads (max_parallel_push) and one for downloads (max_parallel_pull).

[dicej]

Yeah, that sounds great. Update coming shortly.

[flavio]

Overall ok, but I would have preferred to have these configuration knobs added to the ClientConfig struct like @bacongobbler suggested. In this way we would not have needed the new methods of Client to be added.

[dicej]

Ah, good point. I misread @bacongobbler's comment. I'll address that when I'm back at my keyboard.

[dicej]

I've pushed an update. Note that this required a version bump due to a breaking API change (new fields on the ClientConfig struct mean client code which construct instances of that struct (without using ClientConfig::default()) will now need to specify those new fields).