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

perf: Optimize async method allocations #328

Merged
merged 1 commit into from
Dec 5, 2019

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Nov 24, 2019

This is a rework of a closed PR from corefx, dotnet/corefx#37254 and I wrote a small essay explaining why it was a good idea in the initial post for that so it's worth clicking through and reading it.

Profiling the DataAccessPerformance project which emulates the TechEmpower fortunes benchmark you can see that some of the top allocations are state closures for async methods like ReadAsync GetFieldValueAsync. Investigating this I found the code to be much more complex than I'd expected and quite confusing. In order to reduce the allocations I reworked the async infrastructure to use concrete classes and then cached some which are used repeatedly.

Before:
master

After:
branch

DataAccessPerformance results:

name sync threads TPS stdev description
ado-sqlclient+async+128 async 128 64667 3262 master
ado-sqlclient+async+64 async 64 69262 1592 master
ado-sqlclient+async+32 async 32 68814 1918 master
ado-sqlclient+async+128 async 128 67288 1595 branch
ado-sqlclient+async+64 async 64 68870 2395 branch
ado-sqlclient+async+32 async 32 68311 1949 branch

Which shows a ~5% throughput improvement in situations where CPU time is a limitation and less improvements where it isn't a limitation. I recommend reviewing manually because the diff makes the large number of changes and the extraction of lambdas look particularly confusing.

@cheenamalhotra cheenamalhotra added the 📈 Performance Issues that are targeted to performance improvements. label Nov 29, 2019
Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

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

I like the new structure better and I can't see anything obviously wrong. But I'll admit, it's hard to follow async logic in here and the changes are significant enough to not compare well. If we get this in now rather than later, hopefully we'll have time to catch any regressions that might slip through the tests. I'm impressed that you can get your head wrapped around the details well enough to bite off these perf optimizations. I appreciate the work you put into them!

@cheenamalhotra cheenamalhotra added this to the 1.2.0-preview1 milestone Dec 5, 2019
@cheenamalhotra cheenamalhotra merged commit 2a8f998 into dotnet:master Dec 5, 2019
@Wraith2 Wraith2 deleted the perf-asyncresume branch December 6, 2019 11:40
@cheenamalhotra cheenamalhotra added the NetFx 👈 NetCore Issues that require porting from .NET Core to .NET Framework label Apr 1, 2020
cheenamalhotra added a commit to cheenamalhotra/SqlClient that referenced this pull request May 25, 2021
cheenamalhotra added a commit to cheenamalhotra/SqlClient that referenced this pull request May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📈 Performance Issues that are targeted to performance improvements. NetFx 👈 NetCore Issues that require porting from .NET Core to .NET Framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants