[Azure Service Bus] Remove unnecessary LINQ on AmqpReceiver

[danielmarbach]

Removes LINQ usage from the AmqpReceiver from the hot path to improve speed and allocations

I tried to document reasoning inline

Results

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i7-8550U CPU 1.80GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.1.201
  [Host]    : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
  MediumRun : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT

Job=MediumRun  IterationCount=15  LaunchCount=2  
WarmupCount=10  

Method	Elements	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
Before	1	829.1 ns	50.23 ns	75.18 ns	1.00	0.00	0.0362	-	-	152 B
After	1	454.1 ns	62.09 ns	91.01 ns	0.55	0.13	0.0286	-	-	120 B
Before	2	693.1 ns	111.54 ns	166.95 ns	1.00	0.00	0.0401	-	-	168 B
After	2	567.6 ns	90.46 ns	129.74 ns	0.87	0.29	0.0324	-	-	136 B
Before	4	846.9 ns	25.89 ns	38.74 ns	1.00	0.00	0.0458	-	-	200 B
After	4	776.0 ns	32.62 ns	44.65 ns	0.91	0.07	0.0401	-	-	168 B
Before	8	1,864.3 ns	228.77 ns	328.10 ns	1.00	0.00	0.0610	-	-	264 B
After	8	1,484.4 ns	136.14 ns	186.34 ns	0.81	0.13	0.0553	-	-	232 B
Before	16	3,529.6 ns	459.86 ns	674.05 ns	1.00	0.00	0.0916	-	-	392 B
After	16	3,158.8 ns	430.75 ns	631.38 ns	0.91	0.21	0.0839	-	-	360 B

Benchmark used https://github.com/danielmarbach/MicroBenchmarks/blob/438dac249a7a44e178b758e860cf16ca834b0c79/MicroBenchmarks/Linq/LinqGuidConversionContains.cs

[azuresdkci]

Can one of the admins verify this patch?

[pakrym]

Is there a benchmark/profile showing the improvement?

[danielmarbach]

@pakrym happy to provide that if you think this is worth pursuing. I'll revert first the other changes @ShivangiReja pointed out

[pakrym]

@pakrym happy to provide that if you think this is worth pursuing.

It's always good to have an idea of before/after and how the improvement compares to overall performance.

[danielmarbach]

@pakrym added the benchmark results with the changed code that hopefully also addresses the other comments. I ended up with a combination of LINQ and foreach to avoid the Any which leads to very good results.

I'm executing a medium run as I post this and will update the post as soon as I have the results

I'll leave it up to you if you want to take this or not. I have no problems closing this.

[ShivangiReja]

Changes look good to me! Thank you @danielmarbach!

@pakrym Could you please take a look?

[pakrym]

Why not get rid of this piece of linq too?

[danielmarbach]

I did an attempt to do it by creating an array of guid. I then needed to ask for the count on the enumerable which lead to less allocations as the original version but worse performance

[AlexanderSher]

The best gain in the test results above is for the array of 4 elements and it is 175 ns. How many times this method is called?

[JoshLove-msft]

/azp run net - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JoshLove-msft]

@danielmarbach thanks for these performance fixes! They are greatly appreciated 😄 Out of curiosity, have you profiled any end to end scenarios like receiving or sending to identify any bottlenecks? If so, we would be interested to learn about your findings.

[JoshLove-msft]

/azp run net - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AlexanderSher]

I'm sorry for being stubborn, but I'm still interested in the scenario we're optimizing. I've done a simple benchmark based on the danielmarbach/MicroBenchmarks@7a474da#diff-5d50ca96fc4df16892b002db1d70e8c3:

private static async Task<Guid[]> LinqWithAwait(IEnumerable<string> lockTokens)
{
    var lockTokenGuids = lockTokens.Select(token => new Guid(token)).ToArray();
    if (lockTokenGuids.Any(lockToken => _requestResponseLockedMessages.Contains(lockToken)))
        return await Task.FromResult(lockTokenGuids).ConfigureAwait(false);

    return await Task.FromResult(Array.Empty<Guid>()).ConfigureAwait(false);
}

private static async Task<Guid[]> ForeachWithAwait(IEnumerable<string> lockTokens)
{
    var requestResponse = false;
    var lockTokenGuids = lockTokens.Select(token => new Guid(token)).ToArray();
    foreach (var tokenGuid in lockTokenGuids)
    {
        if (!requestResponse && _requestResponseLockedMessages.Contains(tokenGuid)) requestResponse = true;
    }

    if (requestResponse)
    {
        return await Task.FromResult(lockTokenGuids).ConfigureAwait(false);
    }

    return await Task.FromResult(Array.Empty<Guid>()).ConfigureAwait(false);
}

private static Task<Guid[]> LinqWithoutAwait(IEnumerable<string> lockTokens)
{
    var lockTokenGuids = lockTokens.Select(token => new Guid(token)).ToArray();
    if (lockTokenGuids.Any(lockToken => _requestResponseLockedMessages.Contains(lockToken)))
        return Task.FromResult(lockTokenGuids);

    return Task.FromResult(Array.Empty<Guid>());
}

Here are the results:

Method	Job	Runtime	Elements	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
LinqWithAwait	.NET 4.8	.NET 4.8	4	2,767.8 ns	55.45 ns	118.16 ns	1.00	0.00	0.0687	-	-	449 B
ForeachWithAwait	.NET 4.8	.NET 4.8	4	2,708.8 ns	54.26 ns	115.64 ns	0.98	0.06	0.0648	-	-	417 B
LinqWithoutAwait	.NET 4.8	.NET 4.8	4	2,608.3 ns	52.26 ns	147.39 ns	0.95	0.07	0.0572	-	-	369 B
LinqWithAwait	.NET Core 2.1	.NET Core 2.1	4	1,933.2 ns	38.37 ns	71.11 ns	1.00	0.00	0.0648	-	-	416 B
ForeachWithAwait	.NET Core 2.1	.NET Core 2.1	4	1,870.9 ns	37.34 ns	107.74 ns	0.97	0.05	0.0572	-	-	384 B
LinqWithoutAwait	.NET Core 2.1	.NET Core 2.1	4	1,997.0 ns	54.25 ns	159.97 ns	0.98	0.06	0.0534	-	-	344 B
LinqWithAwait	.NET Core 3.1	.NET Core 3.1	4	1,061.0 ns	23.19 ns	68.36 ns	1.00	0.00	0.0515	-	-	416 B
ForeachWithAwait	.NET Core 3.1	.NET Core 3.1	4	986.7 ns	19.76 ns	57.63 ns	0.94	0.08	0.0477	-	-	384 B
LinqWithoutAwait	.NET Core 3.1	.NET Core 3.1	4	974.0 ns	19.89 ns	55.46 ns	0.92	0.06	0.0439	-	-	344 B

As you can see, simple removal of async/await in private method provides better results than LINQ replaced with foreach, both in speed and memory allocation. Yet we are still talking about nanoseconds and bytes here. This code can be improved even further, with total removal of LINQ and casting to array so that foreach doesn't allocate any memory, but will anyone see these optimizations in the shadow of DisposeMessageRequestResponseAsync?

[danielmarbach]

@pakrym I added the break version, new results

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i7-8550U CPU 1.80GHz (Kaby Lake R), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.1.201
  [Host]    : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT
  MediumRun : .NET Core 3.1.3 (CoreCLR 4.700.20.11803, CoreFX 4.700.20.12001), X64 RyuJIT

Job=MediumRun  IterationCount=15  LaunchCount=2  
WarmupCount=10  

Method	Elements	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
Before	1	829.1 ns	50.23 ns	75.18 ns	1.00	0.00	0.0362	-	-	152 B
After	1	454.1 ns	62.09 ns	91.01 ns	0.55	0.13	0.0286	-	-	120 B
Before	2	693.1 ns	111.54 ns	166.95 ns	1.00	0.00	0.0401	-	-	168 B
After	2	567.6 ns	90.46 ns	129.74 ns	0.87	0.29	0.0324	-	-	136 B
Before	4	846.9 ns	25.89 ns	38.74 ns	1.00	0.00	0.0458	-	-	200 B
After	4	776.0 ns	32.62 ns	44.65 ns	0.91	0.07	0.0401	-	-	168 B
Before	8	1,864.3 ns	228.77 ns	328.10 ns	1.00	0.00	0.0610	-	-	264 B
After	8	1,484.4 ns	136.14 ns	186.34 ns	0.81	0.13	0.0553	-	-	232 B
Before	16	3,529.6 ns	459.86 ns	674.05 ns	1.00	0.00	0.0916	-	-	392 B
After	16	3,158.8 ns	430.75 ns	631.38 ns	0.91	0.21	0.0839	-	-	360 B

[danielmarbach]

@AlexanderSher It is true that in comparison to an IO-bound method this could be seen as unnecessary because one could argue the linq version could have better readability. What I have seen though for example with rabbitmq when fetching/sending messages under load those bytes can count.

@JoshLove-msft I rebased to get your other fixes. For other PRs is there a way for me to have a full run done as part of the PR? It seems my previous PR was green but did cause problems when the pipeline run on master.

[JoshLove-msft]

@JoshLove-msft I rebased to get your other fixes. For other PRs is there a way for me to have a full run done as part of the PR? It seems my previous PR was green but did cause problems when the pipeline run on master.

Unfortunately, this would only be caught by the live tests which won't run automatically. I think it is on us at Microsoft to make sure we run the live tests before merging.

[JoshLove-msft]

/azp run net - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JoshLove-msft]

It looks like all the comments have been settled. I'm planning on merging this in assuming the live tests pass.

[JoshLove-msft]

@danielmarbach looks like there is a conflict now with something that I merged in. Sorry about that. Would you mind resolving this?

[danielmarbach]

Rebased

[JoshLove-msft]

/azp run net - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).