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: Add multipacket async read target buffer caching #285

Merged
merged 2 commits into from Nov 7, 2019

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Oct 24, 2019

closes #245

Async reads that span multiple packets can allocate large target arrays and drop them repeatedly causing a large memory and cpu problem. This PR uses the async snapshot already present for all async calls to allow storing the presized target buffer between packet receipt decode attempts avoiding re-allocations.

Before:

Method Behavior Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
Async Default 448.84 ms 8.9453 ms 8.7855 ms 309000.0000 309000.0000 309000.0000 3.98 KB
Sync Default 17.46 ms 0.1025 ms 0.0959 ms 406.2500 406.2500 406.2500 10244.82 KB
Async SequentialAccess 458.23 ms 9.0577 ms 10.4309 ms 310000.0000 310000.0000 310000.0000 3.98 KB
Sync SequentialAccess 17.38 ms 0.0638 ms 0.0596 ms 406.2500 406.2500 406.2500 10244.82 KB

After:

Method Behavior Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
Async Default 124.68 ms 0.7149 ms 0.6687 ms 800.0000 400.0000 200.0000 3.95 KB
Sync Default 16.41 ms 0.0898 ms 0.0840 ms 406.2500 406.2500 406.2500 10244.82 KB
Async SequentialAccess 125.36 ms 1.3684 ms 1.2800 ms 750.0000 500.0000 250.0000 3.95 KB
Sync SequentialAccess 16.48 ms 0.0525 ms 0.0491 ms 406.2500 406.2500 406.2500 10244.82 KB

BDN isn't showing LOH allocations which is why the memory numbers look odd. Using profiler results you can see the savings.

Before:
beforePNG

After:
after

Overall 3x faster, 400 times less GC, 160 times less memory used on the benchmark provided in the report by @roji

@Wraith2 Wraith2 changed the title Perf: Add multipacket async read target buffer caching perf: Add multipacket async read target buffer caching Oct 24, 2019
@roji
Copy link
Member

roji commented Nov 6, 2019

BDN isn't showing LOH allocations which is why the memory numbers look odd. Using profiler results you can see the savings.

As noted in #245 (comment), we found out that it isn't about LOH allocations, but rather that allocations in other threads aren't taken into account. BDN 0.15.0 fixed this.

@David-Engel David-Engel added this to the 1.1.0 milestone Nov 6, 2019
@David-Engel
Copy link
Contributor

@Wraith2 Can you resolve the conflicts and merge the latest from master?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Nov 7, 2019

Done

@David-Engel David-Engel merged commit c3db206 into dotnet:master Nov 7, 2019
@Wraith2 Wraith2 deleted the perf-asyncbytes branch November 8, 2019 22:36
@Suchiman
Copy link
Contributor

@roji BDN prior to that version uses GetAllocatedBytesForCurrentThread which also does not track LOH allocations correctly, see https://github.com/dotnet/coreclr/issues/27691#issuecomment-550305314

@roji
Copy link
Member

roji commented Nov 13, 2019

@Suchiman thanks. I'm not sure if the discrepancies in this particular case were about the LOH allocations or about allocations in other threads, but it doesn't matter much.

@cheenamalhotra cheenamalhotra added this to In progress in SqlClient v1.1.0 via automation Nov 19, 2019
@cheenamalhotra cheenamalhotra moved this from In progress to Done in SqlClient v1.1.0 Nov 19, 2019
@cheenamalhotra cheenamalhotra added the 📈 Performance Use this label for performance improvement activities label Nov 19, 2019
panoskj added a commit to panoskj/SqlClient that referenced this pull request Aug 26, 2022
panoskj added a commit to panoskj/SqlClient that referenced this pull request Nov 21, 2022
panoskj added a commit to panoskj/SqlClient that referenced this pull request Nov 21, 2022
panoskj added a commit to panoskj/SqlClient that referenced this pull request Nov 21, 2022
panoskj added a commit to panoskj/SqlClient that referenced this pull request Sep 24, 2023
panoskj added a commit to panoskj/SqlClient that referenced this pull request Sep 25, 2023
panoskj added a commit to panoskj/SqlClient that referenced this pull request Sep 25, 2023
panoskj added a commit to panoskj/SqlClient that referenced this pull request Oct 25, 2023
panoskj added a commit to panoskj/SqlClient that referenced this pull request Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📈 Performance Use this label for performance improvement activities
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Reading large fields asynchronously is extremely slow
5 participants