Performance: handle non-configured policy callbacks as null (reduce allocations)

[reisenberger]

Many Polly policies allow callback delegates to be configured, eg onRetry, onBreak, onBulkheadRejected. When these are not configured, Polly currently handles this internally by substituting an empty delegate { }.

This issue proposes non-configured callbacks instead be handled internally as null, with eg onRetry?.Invoke(...) (and async equivalent).

The change saves allocations of 64 bytes per execution for some execution cases. (There are also execution-speed improvements, but in the order of nanoseconds - mostly marginal compared to the kind of I/O work Polly normally guards.)

The change will also play more nicely with new syntax proposals.

---

Detail

A comment here proposed non-configured callbacks would be better handled as an empty method. A simple BenchmarkDotNet benchmark suggests this is not so:

public class BenchmarkLambdaMethodLocalFunctionInlining
{
    public static void Execute(Action action)
    {
        action?.Invoke();
    }

    [Benchmark(Baseline = true)]
    public void AllocationNullToActionV0()
    {
        Action doNothingAction = null;

        Execute(doNothingAction);
    }

    [Benchmark]
    public void AllocationToActionV0()
    {
        // ReSharper disable once ConvertToLocalFunction
        Action doNothingAction = () => { };

        Execute(doNothingAction);
    }

    [Benchmark]
    public void LocalFunctionV0()
    {
        void DoNothingLocalFunction()
        {
        }

        Execute(DoNothingLocalFunction);
    }

    [Benchmark]
    public void DeclaredMethodV0()
    {
        Execute(DoNothingDeclaredMethodV0);
    }

    private void DoNothingDeclaredMethodV0()
    {
    }
}

Results:

|                   Method |      Mean |     Error |    StdDev |    Median | Ratio | RatioSD |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------- |----------:|----------:|----------:|----------:|------:|--------:|-------:|------:|------:|----------:|
| AllocationNullToActionV0 | 0.0101 ns | 0.3206 ns | 0.0176 ns | 0.0000 ns |     ? |       ? |      - |     - |     - |         - |
|     AllocationToActionV0 | 2.7876 ns | 0.4159 ns | 0.0228 ns | 2.7826 ns |     ? |       ? |      - |     - |     - |         - |
|          LocalFunctionV0 | 9.7004 ns | 7.1749 ns | 0.3933 ns | 9.5017 ns |     ? |       ? | 0.0153 |     - |     - |      64 B |
|         DeclaredMethodV0 | 9.3043 ns | 4.6542 ns | 0.2551 ns | 9.4288 ns |     ? |       ? | 0.0153 |     - |     - |      64 B |

(the ? in the results is BenchmarkDotNet telling us the actual execution time of the baseline null case was too close to zero (strictly, too close to the benchmark overhead) for the ratios to be considered accurate to calculate)

Both an empty local function and empty declared method cost an extra 64 bytes. It appears that the compiler/JITter can inline the empty delegate { } in this case, but not a pointer to an empty method/local function.

Interestingly, there is no allocation improvement (in this benchmark) between a null and an empty { }. It appears the empty delegate { } in the benchmark can be inlined enough to remove the allocation. However, when the same change is applied to the more complex Polly codebase (always test real code, not only artificial benchmarks 🙂 ), some policy-executions do consistently save 64 bytes of allocs per execution by changing from { } to null.

Handling non-configured callbacks as null will also play more nicely with new syntax proposals.

[reisenberger]

Implemented by #742.

[grant-d]

Interesting. Thanks for following up on this @reisenberger, I didn't expect that result but it makes sense now.