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

Performance regression in ConcurrentDictionary #27440

Closed
sebastienros opened this issue Sep 21, 2018 · 17 comments
Closed

Performance regression in ConcurrentDictionary #27440

sebastienros opened this issue Sep 21, 2018 · 17 comments
Milestone

Comments

@sebastienros
Copy link
Member

From Benchview, 90% regression on IsEmpty and 40% on TryGetValue, around August 19th.

benchview link

The impact is visible in ASP.NET on the scenarios that rely on ConcurrentDictionary like routing and response caching, by about 10%.

@AndyAyersMS
Copy link
Member

The timing is about right for this to be tiered compilation.

@sebastienros
Copy link
Member Author

My first thought too, so I asked @kouvel first. I thought it was because of the only recent change on IsEmpty, but apparently it is not (from benchview). I know how I could enable tiered before it's the default, but if I knew how to disable it, then I could try to run a benchmark without it and see if it changes.

@AndyAyersMS
Copy link
Member

You can disable with COMPlus_TieredCompilation=0. Or see our handy new guide.

@sebastienros
Copy link
Member Author

I ran a benchmark on the scenario that regressed in ASP.NET MVC, and disabling TC doesn't make it better, so it doesn't look like this is the reason.

TC=1: 484,487 RPS (and better startup time + first request obviously)
TC=0: 476,111 RPS

For anyone interested I can share the etl traces before and after the regression, that show TryGetValue as the reason.

@danmoseley
Copy link
Member

Does it align with any changes in ConcurrentDictionary - I'm not sure whether you're saying it is possibly the IsEmpty change, or definitely not. If it is possibly that change, can you try with it reversed?

cc @tarekgh

@stephentoub
Copy link
Member

Does it align with any changes in ConcurrentDictionary

No. IsEmpty was modified back on June 2nd, but:
a) That's well before Aug 19th.
b) That didn't affect TryGetValue at all (TryGetValue doesn't use IsEmpty)
c) The change is fairly small and is meant to improve the case where the dictionary isn't empty (by avoiding taking a lock), and when it is empty, the extra work done compared to taking the lock is minimal, so even if that did regress in a microbenchmark on IsEmpty, the overall impact should be extremely minimal (and it's really, really hard to see that regressing even the microbenchmark by 90%, considering our own runs of the microbenchmark showed nothing of the sort).

And TryGetValue hasn't been touched in years as far as I can tell.

I'm unable to get to the provided benchmark link.

@sebastienros
Copy link
Member Author

@stephentoub open the link on Edge. TLS version issues IFAIR.

@stephentoub
Copy link
Member

Thanks. That worked. From those graphs, the IsEmpty test that suffered was when the dictionary wasn't empty, which makes it even less likely to be related to that change.

I think it's more likely to be JIT related. Are there are any other tests across all suites that took a hit around then?

@stephentoub
Copy link
Member

Also, am I reading the graphs right that the most recent runs have eliminated most of the regression?

@kouvel
Copy link
Member

kouvel commented Sep 25, 2018

The IsEmpty regression [edit: in benchview] is most likely due to tiered compilation being enabled by default. With the change to use BenchmarkDotNet, this class of regressions should go away. The issue @sebastienros is seeing is something different, we're working on determining what that is.

@sebastienros
Copy link
Member Author

I ran another benchmark with Tiered on and off on our Response Caching scenario which uses a MemoryCache backed by a CD. This time I can repro the regression and get the same numbers as before. I am using this scenario because a recent change in Routing is removing some usage of the concurrent dictionary, so the numbers I gave previously are invalid.

So yes, it looks like a side effect of Tiered compilation.

@kouvel
Copy link
Member

kouvel commented Sep 25, 2018

@sebastienros, did I understand correctly that the Response Caching scenario is performing worse with tiered compilation on compared with tiered compilation off on the same runtime build?

There are some known perf issues with tiering, in particular when it comes to benchmarking https://github.com/dotnet/coreclr/issues/19751 is an issue that would likely be hit. Is it possible that this could be an issue in the way the perf harness measures perf?

Are there a repeatable set of steps I can follow to reproduce the results you're seeing?

@sebastienros
Copy link
Member Author

@kouvel I am sending the repro by email

@sebastienros
Copy link
Member Author

/cc @Eilon

@Eilon
Copy link
Member

Eilon commented Nov 15, 2018

@sebastienros / @kouvel - so of course this doesn't matter for 2.2 anymore, but is there a follow-up investigation that needs to occur for 3.0?

@kouvel
Copy link
Member

kouvel commented Nov 19, 2018

Yes I'm currently working on fixing https://github.com/dotnet/coreclr/issues/19752 for 3.0, that should cover the regressions that were seen

@kouvel
Copy link
Member

kouvel commented Dec 18, 2018

Keeping https://github.com/dotnet/coreclr/issues/19752 as the primary issue and closing this one as duplicate

@kouvel kouvel closed this as completed Dec 18, 2018
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants