Caching access to virtual properties

[vonzshik]

Reading from virtual properties in a cycle/sequentially is very slow because the compiler can't cache the returned value from the property (because there might be side effects). This pr helps to alleviate this problem for some of the properties (the worst offender is QueryContext.Context, which is called every time a new MaterializationContext is created, but changing this is not trivial). Below is the benchmark results of reading from fields and properties in a cycle of 10.

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
AMD Ryzen 5 2400G with Radeon Vega Graphics, 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=6.0.100-preview.3.21202.5
  [Host]     : .NET Core 6.0.0 (CoreCLR 6.0.21.20104, CoreFX 6.0.21.20104), X64 RyuJIT
  DefaultJob : .NET Core 6.0.0 (CoreCLR 6.0.21.20104, CoreFX 6.0.21.20104), X64 RyuJIT

Method	Mean	Error	StdDev
Field	2.885 ns	0.0515 ns	0.0456 ns
Property	2.895 ns	0.0644 ns	0.0537 ns
VirtualProperty	17.204 ns	0.3340 ns	0.2961 ns
VirtualPropertyCached	4.847 ns	0.0433 ns	0.0384 ns

benchmark code

public class VirtualPropertyBenchmark
{
	public int ValueField;

	public int ValueProperty { get; set; }

	public virtual int ValuePropertyVirtual { get; set; }

	[GlobalSetup]
	public void Setup()
	{
		ValueField = 42;
		ValueProperty = 42;
		ValuePropertyVirtual = 42;
	}

	[Benchmark]
	public int Field()
	{
		var valueSum = 0;
		for (var i = 0; i < 10; i++)
		{
			valueSum += ValueField;
		}

		return valueSum;
	}

	[Benchmark]
	public int Property()
	{
		var valueSum = 0;
		for (var i = 0; i < 10; i++)
		{
			valueSum += ValueProperty;
		}

		return valueSum;
	}

	[Benchmark]
	public int VirtualProperty()
	{
		var valueSum = 0;
		for (var i = 0; i < 10; i++)
		{
			valueSum += ValuePropertyVirtual;
		}

		return valueSum;
	}

	[Benchmark]
	public int VirtualPropertyCached()
	{
		var valueSum = 0;
		var value = ValuePropertyVirtual;
		for (var i = 0; i < 10; i++)
		{
			valueSum += value;
		}

		return valueSum;
	}
}

On fortunes benchmark this change got me around 50 ms/minute.

cc @roji

[dnfadmin]

All CLA requirements met.

[roji]

I admit I'm surprised - this simple change seems to get us a 1% increase in RPS... Seems like we're at a point where virtual dispatch starts to make a difference?

We should do the same in SplitQueryingEnumerable, and possibly also look at FromSqlQueryingEnumerable (for DbDataReader) and Cosmos.

[roji]

Single ResultContext is read-only on SingleQueryResultCoordinator, we could cache it as a field on SingleQueryingEnumerable rather than doing it on each MoveNext, no?

[smitpatel]

Should we just make our result coordinators sealed and non-virtual properties? They are public and overridable because of pub-ternal philosophy. It it is hurting perf then I don't think it is worth sticking to it.

[roji]

Sure, we can go that way too. Though we're accessing some other virtual properties in MoveNext which we should probably cache (DbDataReader, CancellationToken), so we'd go down the caching route anyway...

[AndriySvyryd]

We should make members on QueryContext and derived classes non-virtual too.

[roji]

@AndriySvyryd yeah.. I pushed a commit which at least caches QueryContext.Context so it doesn't get recalculated all the time.

[roji]

Pushed a commit eliminating some more virtual lookups in the QueryingEnumerables.

[smitpatel]

No need of _resultContext. Make whole structure for co-ordinating results non-virtual or leave them I can take care of it in future.

_cancellationToken/_dbDataReader are fine.

[roji]

Thanks again for this @vonzshik!!