Add INatsConnectionPool interface

[jasper-d]

#105

[jasper-d]

Opted for an explicit interface implementation so clients could still call the non-virtual GetConnection if they are concerned about perf. That being said, I am almost certain that at least the virtual call to GetConnection could be inlined with GDV in .NET 8.

[mtmk]

I haven't looked into the hosting APIs properly yet. Would this be on a hot path?

[jasper-d]

Not for the DI integration in NATS.Client.Hosting. It's always resolving NatsConnectionPool and than calling the non-virtual GetConnection() method.
Everything else depends on usage, i.e. if NatsConnection or INatsConnection is injected.
If people have DI resolution on their hotpath they have bigger performance issues than some virtual calls I suppose.

The only benefit here is when directly using NatsConnectionPool and frequently invoking a method on the returned I/NatsConnection. If we don't use an explicit interface those would be virtual calls, e.g. something like this:

var conn = connPool.GetConnection();
while(true)
{
  // This would either be non-virtual if conn was retrieved from
  // NatsConnectionPool and virtual if it was retrieved from INatsConnectionPool
  await conn.PublishAsync(); 
}

But I'm not convinced this is actually worth the ugliness of explicit interface impl.

[mtmk]

Looks like still using callvirt:

https://sharplab.io/#v2:C4LglgNgPgAgzAAjAO2AUwE4DMCGBjNBASQQG8EYBGABgQFkAKASgG4EBfAWACh4KAmBAGEEIYmR4IpFGsQB0jJggC8APnrMWk6Xyq1FK9VTkBRZADcwGAPbIAtmlRyA4mmBnLN+4+AA1HBhgOABGEGgMAEQAGhFMAIRa3Fy8iDCCAAoS3NIUqQAs9AD6lAwieEpqCHgKmtpSugV0hfwMJGAV6mA1rDzJQA=

https://sharplab.io/#v2:C4LglgNgPgAgzAAjAO2AUwE4DMCGBjNBAFQQG8EYAWBAYQAoBKAbgQF8BYAKHiVU1wIIAkmWIIA4mmBFGLDt0QwATLQQhhZLgm1iA+sCZad9EsAYIAvAD4E+ywgNHtJIQDpJ0xpZseZzJxSIJL5e1raOnPI8yggACpqcOoEU1ACyugCMdDQIeOakAUkAbjgYDvZ47lJ+holJOgDuABaQaHTAGACuaPmF9drArvT+df3ySeM60Wm6SnQiYL2j9SVlwPZgVZ4j/drNre1dPQm79YPDtaeT2vLyQA==

Unless I'm missing the point of course.

[jasper-d]

It's callvirt in MSIL, but since JIT sees the exact type in M_2, it uses a uses a non-virtual call. I find it easier to see in Compiler Explorer: https://godbolt.org/z/fYEnxEqo9

Here, C.M() got inlined which only happens because the call is non-virtual.

[jasper-d]

But for me it's fine either way, only having those methods in the interface or having the explicit interface implementation in addition to the methods on the class.

[mtmk]

It's callvirt in MSIL, but since JIT sees the exact type in M_2, it uses a uses a non-virtual call. I find it easier to see in Compiler Explorer: https://godbolt.org/z/fYEnxEqo9

I see. However commenting out explicit implementation isn't making any difference https://godbolt.org/z/6Y8K9E3vf

[mtmk]

But for me it's fine either way, only having those methods in the interface or having the explicit interface implementation in addition to the methods on the class.

We do care a lot about performance and this won't make any functional difference here, so if you think there may still be benefit, please add a few lines of comment explaining why we used an explicit interface here.

[jasper-d]

However commenting out explicit implementation isn't making any difference

The "problem" is, that w/o the explicit implementations, clients can't obtain a NatsConnection instance but only an INatsConnection and thus all calls on that connection instance will be virtual.

I removed it anyway, because the longer I look at it, the more it feels like premature optimization. It can still be added anytime.

Some figures:

Method	Job	Runtime	Mean	Error	StdDev	Median	Ratio	RatioSD
CallDirect	.NET 6.0	.NET 6.0	1.0188 ns	0.0365 ns	0.0342 ns	1.0136 ns	1.00	0.00
CallVirtual	.NET 6.0	.NET 6.0	2.6844 ns	0.0399 ns	0.0373 ns	2.6744 ns	2.64	0.10
CallDirect	.NET 7.0	.NET 7.0	0.5650 ns	0.0365 ns	0.0324 ns	0.5700 ns	1.00	0.00
CallVirtual	.NET 7.0	.NET 7.0	2.1274 ns	0.0560 ns	0.0437 ns	2.1451 ns	3.75	0.23
CallDirect	.NET 8.0	.NET 8.0	0.2981 ns	0.0077 ns	0.0068 ns	0.2977 ns	1.00	0.00
CallVirtual	.NET 8.0	.NET 8.0	0.3829 ns	0.0284 ns	0.0251 ns	0.3783 ns	1.28	0.08
CallDirect	NativeAOT 7.0	NativeAOT 7.0	1.5757 ns	0.0573 ns	0.0926 ns	1.6269 ns	1.00	0.00
CallVirtual	NativeAOT 7.0	NativeAOT 7.0	1.1989 ns	0.0529 ns	0.0650 ns	1.2314 ns	0.78	0.06
CallDirect	NativeAOT 8.0	NativeAOT 8.0	0.3246 ns	0.0153 ns	0.0143 ns	0.3159 ns	1.00	0.00
CallVirtual	NativeAOT 8.0	NativeAOT 8.0	0.5110 ns	0.0382 ns	0.0886 ns	0.5193 ns	1.42	0.39

Benchmark code

[SimpleJob(RuntimeMoniker.Net60)]
[SimpleJob(RuntimeMoniker.Net70)]
[SimpleJob(RuntimeMoniker.Net80)]
[SimpleJob(RuntimeMoniker.NativeAot70)]
[SimpleJob(RuntimeMoniker.NativeAot80)]
public class Calls
{
    public interface IFoo
    {
        int Do();
    }

    public sealed class Foo : IFoo
    {
        public int Do() => 42;
    }
    
    public static IFoo _virtualFoo = new Foo();
    public static Foo _directFoo = new Foo();

    [Benchmark(Baseline = true)]
    public int CallDirect()
    {
        return Do(_directFoo);

        [MethodImpl(MethodImplOptions.NoInlining)]
        static int Do(Foo foo)
        {
            return foo.Do();
        }
    }
    
    [Benchmark]
    public int CallVirtual()
    {
        return Do(_virtualFoo);

        [MethodImpl(MethodImplOptions.NoInlining)]
        static int Do(IFoo foo)
        {
            return foo.Do();
        }
    }
}

[mtmk]

Quite a difference in benchmark results, double, triple in some cases. Definitely we must use it on hot paths. But I agree with you maybe not necessary for this scenario. Thanks for your hard work @jasper-d we really do appreciate your contribution.

[mtmk]

btw @jasper-d please sign your commits.

[caleblloyd]

Are we definitely set on making moving these to NatsConnection? Or should we try to convert them to extension methods that extend INatsConnection instead?

The extension methods could be nice for someone else who implements INatsConnection but I'm not sure if that will be common

[jasper-d]

I've made changes to keep the four extension methods that don't need to be instance members as extension methods and only moved those four to the interface that access internal methods (i.e. RequestSubAsync).

I think outside of tests it's highly unlikely that anyone want's to implement INatsConnection.

[mtmk]

LGTM @caleblloyd can you have a look as well please?

[caleblloyd]

Looks good, I think that the Request-Reply methods should probably either All be in the interface, or All be in extensions of INatsConneciton. But we can make a decision on that and move those around in a follow-up