Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct defect with concurrency benchmarks (and add nested scope benchmark) #1062

Merged
merged 1 commit into from
Dec 21, 2019

Conversation

alistairjevans
Copy link
Member

When I was adding a new benchmark for testing nested scope concurrency, the concurrency test was taking absolutely forever, plus I got a CLR crash inside the GC, apparently an out of memory error.

I figured out why...it turns out the concurrency benchmark has never tested autofac concurrency, because at the end of the benchmark method, after building it calls:

Task.WhenAll(tasks)

This doesn't block, so it returns immediately, and the GC can't clean up the tasks. The benchmark was only testing how fast we can add tasks to a list.

When we add the appropriate wait:

Task.WhenAll(tasks).Wait()

The concurrency tests run in normal time, and do measure autofac performance.

Here's the updated latest develop benchmarks, including for the new benchmark I added.

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18362
AMD Ryzen 5 2500U with Radeon Vega Mobile Gfx, 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100
  [Host]     : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT
  DefaultJob : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT

Method ResolveTaskCount ResolvesPerTask Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
MultipleResolvesOnMultipleTasks 100 100 1.727 ms 0.0342 ms 0.0407 ms 2542.9688 - - 5.05 MB
Method ConcurrentRequests RepeatCount Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
MultipleResolvesOnMultipleTasks 100 10 3.282 ms 0.0607 ms 0.0568 ms 3484.3750 - - 6.9 MB

I'd suggest we run the updated benchmarks on the pending immutable changes before we merge those over.

Copy link
Member

@alexmg alexmg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch and nice to see another benchmark added to the list!

@alexmg alexmg merged commit 81e22bd into autofac:develop Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants