Implement Hedging (Retry Slow Parts) for Upload

[waahm7]

Description of changes:

• Implements Hedging (retry slow parts) for upload. Based on my experiments, I didn't see any difference for Download, so this PR only adds it for Upload. We can easily add it to Download stack in future if needed.

Here are the benchmarks with and without hedging for Upload:

60GB Upload with 250 concurrency:
With hedging:
Run:1 Secs:16.674105 Gb/s:30.909969
Run:2 Secs:16.062010 Gb/s:32.087895
Run:3 Secs:16.787110 Gb/s:30.701895
Run:4 Secs:15.197997 Gb/s:33.912106
Run:5 Secs:16.218516 Gb/s:31.778250
Run:6 Secs:16.623848 Gb/s:31.003417
Run:7 Secs:16.910915 Gb/s:30.477126
Run:8 Secs:15.788846 Gb/s:32.643048
Run:9 Secs:15.869860 Gb/s:32.476409
Run:10 Secs:16.757620 Gb/s:30.755923
Without Hedging:
Run:1 Secs:17.601898 Gb/s:29.280711
Run:2 Secs:16.084260 Gb/s:32.043505
Run:3 Secs:17.486960 Gb/s:29.473167
Run:4 Secs:17.730397 Gb/s:29.068502
Run:5 Secs:17.425133 Gb/s:29.577741
Run:6 Secs:16.279045 Gb/s:31.660093
Run:7 Secs:16.883302 Gb/s:30.526971
Run:8 Secs:17.831837 Gb/s:28.903140
Run:9 Secs:17.302441 Gb/s:29.787478
Run:10 Secs:17.429089 Gb/s:29.571028

30 GB with 250 concurrency:
With Hedging:
Run:1 Secs:9.298489 Gb/s:27.713968
Run:2 Secs:9.620810 Gb/s:26.785483
Run:3 Secs:8.474216 Gb/s:30.409661
Run:4 Secs:8.710808 Gb/s:29.583712
Run:5 Secs:8.909990 Gb/s:28.922372
Run:6 Secs:8.086036 Gb/s:31.869516
Run:7 Secs:7.887725 Gb/s:32.670770
Run:8 Secs:9.721999 Gb/s:26.506692
Run:9 Secs:8.053054 Gb/s:32.000038
Run:10 Secs:8.440625 Gb/s:30.530680

Without Hedging:
Run:1 Secs:11.358797 Gb/s:22.687090
Run:2 Secs:10.803229 Gb/s:23.853797
Run:3 Secs:11.495138 Gb/s:22.418003
Run:4 Secs:9.150468 Gb/s:28.162280
Run:5 Secs:10.896210 Gb/s:23.650245
Run:6 Secs:10.624905 Gb/s:24.254150
Run:7 Secs:10.698735 Gb/s:24.086777
Run:8 Secs:9.340982 Gb/s:27.587897
Run:9 Secs:11.398385 Gb/s:22.608294
Run:10 Secs:10.350546 Gb/s:24.897047

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[aajtodd]

probably define these as constants and maybe some comments on how/why they were chosen.

[waahm7]

Thanks, I have moved it to constants and added docs.

[aajtodd]

Question: Given this doesn't actually place requests on the wire yet is there a reasonable constant that would make sense here?

[waahm7]

The buffer layer docs mention that

/// When [`Buffer`]'s implementation of [`poll_ready`] returns [`Poll::Ready`], it reserves a
/// slot in the channel for the forthcoming [`call`]. However, if this call doesn't arrive,
/// this reserved slot may be held up for a long time. As a result, it's advisable to set
/// `bound` to be at least the maximum number of concurrent requests the [`Buffer`] will see.
/// If you do not, all the slots in the buffer may be held up by futures that have just called
/// [`poll_ready`] but will not issue a [`call`], which prevents other senders from issuing new
/// requests."

So I thought that the max number of requests was a good value here.

[aajtodd]

Question: Do you need to box this?

[waahm7]

I am not sure how to get rid of the box here. downcast returns the Box<Error>, and the Box::into_inner API is unstable. I had to box it to have the same type in both branches.

[aajtodd]

trivial: You could probably just make this the default policy since downloads would look the same and move it into the hedge module directly. If we ever need to differentiate we could but then you could just do something like:

let svc = ServiceBuilder::new()
    ...
    .layer(hedge::Builder::default().into_layer())

[waahm7]

Thanks, updated it to the default policy.

[graebm]

The extra "hedging" request should count extra towards the concurrency limit. If this is hard to do, let's do it in a followup task.

Even better would be if hedged requests were lower priority, and only got a ticket when there's no other work to do. Since they really only help us if we're done with a workload and all that's left is the slow requests. Otherwise they're just stealing from another request that would be more useful.

[waahm7]

I have looked into this, and it seems like the hedged requests can bypass the concurrency_limit layer. Reorganizing the layers had no effect. I have added a TODO to fix this for now since I want to get this merged and see if there is any difference in benchmarks.

Another interesting thing I noticed is that we are hedging very few requests, like for a 30GB upload with 125 concurrency, we only hedged less 5-15 requests. We can revisit the numbers once we have more tracing/metrics in place.