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 regressions on EF scenarios #36292

Closed
sebastienros opened this issue May 12, 2020 · 15 comments
Closed

Performance regressions on EF scenarios #36292

sebastienros opened this issue May 12, 2020 · 15 comments
Labels
Milestone

Comments

@sebastienros
Copy link
Member

Most probably due to threadpool/epoll changes on the same commit.

dotnet/aspnetcore#21593
dotnet/aspnetcore#21596
dotnet/aspnetcore#21634
dotnet/aspnetcore#21640
dotnet/aspnetcore#21650
dotnet/aspnetcore#21688

The lines that are up are either ADO.NET or Dapper. The ones that are down are EF.

image

/cc @adamsitnik @roji

@sebastienros sebastienros self-assigned this May 12, 2020
@Pilchie
Copy link
Member

Pilchie commented May 12, 2020

Should we transfer this to the runtime repo?

@sebastienros
Copy link
Member Author

@Pilchie that's probably where it should be. There is a chance that it gets closed since it's not regressing from the previous version. It's an unfortunate "local" regression. But I am sure some investigation will definitely be done to at least understand the reasons.

@Pilchie Pilchie transferred this issue from dotnet/aspnetcore May 12, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 12, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

@danmoseley
Copy link
Member

@kouvel @adamsitnik

@davidfowl
Copy link
Member

@sebastienros didn't you say it was from @adamsitnik 's change (single epoll thread?). We have diffs to show what changes affect the graph right?

@stephentoub
Copy link
Member

stephentoub commented May 12, 2020

EF looks like it went up for a day or two and then went back down to what it was. Were the few measurements showing a temporary increase legit? Just making sure.

Also, what is the magnitude of the regression? It's hard to tell from the screenshot, but it looks fairly small?

@sebastienros
Copy link
Member Author

sebastienros commented May 12, 2020

Here are the actual runtime versions that regressed, and links to commits. I can't be more precise than that as it's based on the available runtime builds.

Scenario: DbFortunesEF

5.0.0-preview.5.20253.4 154K RPS
5.0.0-preview.5.20253.5 170K RPS
82eff51...2d29378 (contains @kouvel 's changes)

5.0.0-preview.5.20253.7 166K RPS
5.0.0-preview.5.20257.11 151K RPS
8527a99...9334426 (contains @adamsitnik 's changes)

Command line:

dotnet run -- --server http://asp-citrine-lin:5001 --client http://asp-citrine-load:5002 
-j ..\Benchmarks\benchmarks.html.json -n DbFortunesEF --database PostgresQL 
--warmup 15 --duration 30 --self-contained 
--aspnetcoreversion 5.0.0-preview.5.20253.1 --runtimeversion 5.0.0-preview.5.20257.11 

Pinned ASP.NET version across runs.
To collect traces add --collect-trace.
Change the runtime version accordingly.

@ghost
Copy link

ghost commented Jul 6, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@mangod9
Copy link
Member

mangod9 commented Jul 6, 2020

Moving to sockets area, since this is related to epoll changes.

@scalablecory scalablecory added this to the 5.0.0 milestone Jul 7, 2020
@scalablecory scalablecory removed the untriaged New issue has not been triaged by the area owner label Jul 7, 2020
@scalablecory
Copy link
Contributor

Triage: worth an investigation. @antonfirsov @adamsitnik can you help here?

@karelz karelz added the tenet-performance Performance related issue label Jul 27, 2020
@adamsitnik
Copy link
Member

From the numbers provided here, it looks like #35330 has improved the performance compared to 3.1 and #35800 has brought it back to the previous level (for these particular benchmarks). The goal of the first PR was to allow us to switch to a single epoll thread (second PR), which gave us BIG wins in all platform benchmarks. I would say that it was an unplanned improvement which was simply reverted by the other PR that gave us very important wins elsewhere.

@sebastienros Do we have a .yml config file for these benchmarks so I could run them with Driver2 and compare 3.1 vs 5.0?

I am going to set it to the future for now (it does not look like a regression compared to previous release)

@adamsitnik adamsitnik modified the milestones: 5.0.0, Future Aug 4, 2020
@geoffkizer
Copy link
Contributor

I think pushing this out is fine.

That said, while the issue is still fresh in our minds, any ideas what might have caused this @adamsitnik?

@sebastienros
Copy link
Member Author

@adamsitnik
Copy link
Member

@sebastienros thanks for the link, it was exactly what I needed!

I've run the benchmarks for 3.1.5 vs 5.0 using perf machine (12 cores, close to real life scenarios) and Citrine (TechEmpower, 28 cores):

Machine Benchmark 3.1 5.0 "(5.0/3.1) - 1"
perf fortunes (ADO.NET) 97,615 110,035 +12.72%
  fortunes_ef (EF) 67,787 70,534 +4.05%
citrine fortunes (ADO.NET) 233,312 266,329 +14.15%
  fortunes_ef (EF) 151,918 145,658 -4.12%

there are improvements for ADO.NET and one regression for EF for a machine with a high number of cores

any ideas what might have caused this

@geoffkizer If ADO.NET did not improve, I would say that we can't keep the ThreadPool busy with a single epoll thread. But since only EF has regressed for a machine with a high number of cores I guess that something which was not a bottleneck so far, became a bottleneck (lock contention?). We need to run a full investigation to find out (and hopefully change something in EF).

@wfurt
Copy link
Member

wfurt commented Dec 15, 2022

triage: this was open for 5.0 and all the linked issues are closed. Not clear if this is really actionable. We should investigate perf improvements separately.

@wfurt wfurt closed this as completed Dec 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2023
@karelz karelz removed this from the Future milestone Mar 22, 2023
@karelz karelz added this to the 8.0.0 milestone Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests