Perf: add allocation free path for close notifications #1198
Conversation
DavoudEshtehari
requested review from
JRahnama,
Kaur-Parminder and
DavoudEshtehari
Wraith2
force-pushed
the
perf-registerclosenotification
branch
from
054b9c6
to
6286277
Compare
|
@David-Engel can i get this reviewed and merged please. It's been open 2 years now and it's really not a large or complex change. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1198 +/- ##
==========================================
+ Coverage 70.59% 70.99% +0.40%
==========================================
Files 306 306
Lines 61667 64083 +2416
==========================================
+ Hits 43533 45495 +1962
- Misses 18134 18588 +454
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
The only remaining "failures" in the checks are from Windows+.net7, which I disabled in the CI pipeline to eliminate 33 jobs and improve code coverage merge time. The matrix was getting huge. I left .net7 coverage for Linux/macOS since the matrix there isn't so big. The failures were originally from the "Enable TCP, NP & Firewall" task, which is fixed now. |
|
No idea. It might be that scalar is just ExecuteReader wearing a silly hat so the call to close notification may be lower in the call hierarchy. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…e is setup correctly (dotnet#1198)
When a real (not CancellationToken.None) cancellation token is used in an async query a callback is registered against it to make sure that the pending async action can be cancelled correctly. To do this two pieces of information are needed in a continuation and there is only one state parameter so a closure or state object have to be used, unless...
Since we own the task that is being passed into the continuation and in all current cases we are not providing a task state object and we're directly using TaskCompletionSources we can set the TCS state object that is passed to the child to be one of the bits of state that we need and then we can use the state parameter as the other. This is a little tortuous but I've annotated it in the code so it's clear why it's being done. I've also left the original allocating pathway in place so if anyone ever needs that state parameter they can revert and the code will still work just with an extra allocation per invocation.
This saves one tuple allocation per async call when a real cancellation token is used.