Reduce async overhead

[pentp]

Remove many unnecessary async awaits from the pipeline hot path.
Also cleaned up some excessive use of lambdas.

[martincostello]

Thanks for your PR.

I'm not against any improvements that don't impact the public API surface, but ValueTask was specifically adopted during the development of Polly v8 to reduce memory allocations so those parts of the changes raise some questions.

Have you run the latest benchmarks before and after these changes? We'd want to see some concrete numbers on any improvements and trade-offs that these changes might make.

Some parts could potentially be moved out into separate smaller PRs as they're pretty uncontroversial (e.g. using ArgumentNullException.ThrowIfNull(), the unused Stopwatch) so would get merged sooner.

[martincostello]

I've cherry-picked some of the changes here into #2667, which has now been merged.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.12%. Comparing base (f52f37f) to head (631fc87).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2664      +/-   ##
==========================================
- Coverage   96.23%   96.12%   -0.11%     
==========================================
  Files         311      309       -2     
  Lines        7322     7118     -204     
  Branches     1012     1006       -6     
==========================================
- Hits         7046     6842     -204     
  Misses        222      222              
  Partials       54       54              

Flag	Coverage Δ
linux	96.12% <100.00%> (-0.11%)	⬇️
macos	96.12% <100.00%> (-0.11%)	⬇️
windows	96.11% <100.00%> (-0.11%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pentp]

The changes from ValueTask to Task are improvements - removing an async ValueTask wrapping method around a Task.
The other changes are just reducing async overhead - ValueTask is allocation free only for synchronously completed methods (or if using pooling) and even then the async machinery has overhead.

[martincostello]

Have you run the latest benchmarks before and after these changes? We'd want to see some concrete numbers on any improvements and trade-offs that these changes might make.

[pentp]

Some benchmark numbers:

Baseline	Mean	Error	StdDev	Ratio	Gen0	Allocated
Hedging_Primary	483.1 ns	10.93 ns	16.36 ns	1.00	-	-
Hedging_Secondary	850.4 ns	46.46 ns	69.54 ns	1.76	0.0143	240 B
Hedging_Primary_AsyncWork	2,165.3 ns	43.49 ns	62.38 ns	4.49	0.1335	2287 B
Hedging_Secondary_AsyncWork	2,666.8 ns	47.49 ns	69.61 ns	5.53	0.1488	2514 B

PR	Mean	Error	StdDev	Ratio	Gen0	Allocated
Hedging_Primary	308.9 ns	2.98 ns	4.46 ns	1.00	-	-
Hedging_Secondary	553.4 ns	5.53 ns	8.28 ns	1.79	0.0143	240 B
Hedging_Primary_AsyncWork	1,356.6 ns	84.79 ns	126.91 ns	4.39	0.0763	1279 B
Hedging_Secondary_AsyncWork	1,658.0 ns	39.59 ns	59.26 ns	5.37	0.0877	1489 B

Baseline	Mean	Error	StdDev	Ratio	Gen0	Allocated
ExecuteStrategyPipeline_Generic_V7	833.3 ns	5.22 ns	7.65 ns	1.00	0.1640	2744 B
ExecuteStrategyPipeline_Generic_V8	831.1 ns	2.77 ns	4.06 ns	1.00	0.0019	40 B
ExecuteStrategyPipeline_GenericTelemetry_V8	1,139.6 ns	8.02 ns	12.00 ns	1.37	0.0019	40 B
ExecuteStrategyPipeline_NonGeneric_V8	884.4 ns	5.82 ns	8.71 ns	1.06	0.0019	40 B
ExecuteStrategyPipeline_NonGenericTelemetry_V8	1,204.2 ns	6.91 ns	10.35 ns	1.45	0.0019	40 B

PR	Mean	Error	StdDev	Ratio	Gen0	Allocated
ExecuteStrategyPipeline_Generic_V7	800.7 ns	4.52 ns	6.76 ns	1.00	0.1640	2744 B
ExecuteStrategyPipeline_Generic_V8	719.3 ns	2.86 ns	4.29 ns	0.90	0.0019	40 B
ExecuteStrategyPipeline_GenericTelemetry_V8	983.1 ns	3.09 ns	4.62 ns	1.23	0.0019	40 B
ExecuteStrategyPipeline_NonGeneric_V8	821.5 ns	3.29 ns	4.93 ns	1.03	0.0019	40 B
ExecuteStrategyPipeline_NonGenericTelemetry_V8	1,073.5 ns	2.95 ns	4.42 ns	1.34	0.0019	40 B

For actually async flows the improvements are significant, but even just sync flows benefit from the reduced overhead. Unfortunately almost all existing benchmarks are synchronous so the async overhead of many layers of (Value)Tasks isn't very visible.

[martincostello]

Thanks for taking the time to run the benchmarks - it looks like all the numbers have improved for both duration and memory consumption.

[martincostello]

Just one last comment and fixing the mutation tests and this looks good to merge.