Skip to content

Add async snapshot continue capability for multipacket fields #3161

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Mar 14, 2025
Merged

Add async snapshot continue capability for multipacket fields #3161

merged 9 commits into from
Mar 14, 2025

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Feb 19, 2025

Split out from #2608 per discussion detailed in #2608 (comment)

Adds infrastructure to the async reader snapshot to allow the last packet of the snapshot to be captured as a continue point for the snapshot. On replay if a continue point is available it is used which skips re-doing the previous work improving speed.

The name and behaviour of the appcontext switch being used will need agreement and work. This continue capability requires the partial packet capability so legacy mode enabled on partial packets will force this feature to be disables. I expect there to be some discussion around the names and exact details so the current PR is in dev form, easy to see and change.

Benchmarks for reading a 20Mib string from a local sql server. As payload sizes increase and latency increases speed differences should be more noticeable. I invite and request people to do some benchmarking to make sure we're aware of the performance characteristics of this change.

BenchmarkDotNet v0.14.0, Windows 11 (10.0.26100.2314)
.NET SDK 9.0.200
  [Host]     : .NET 8.0.13 (8.0.1325.6609), X64 RyuJIT
  DefaultJob : .NET 8.0.13 (8.0.1325.6609), X64 RyuJIT


| Method | UseContinue | Mean      | Error     | StdDev    | Gen0      | Gen1      | Gen2      | Allocated |
|------- |------------ |----------:|----------:|----------:|----------:|----------:|----------:|----------:|
| Async  | False       | 757.51 ms | 15.053 ms | 36.642 ms | 2000.0000 | 1000.0000 |         - | 101.49 MB |
| Sync   | False       |  39.40 ms |  0.543 ms |  0.508 ms | 2000.0000 |  888.8889 |  777.7778 |  80.14 MB |
| Async  | True        |  49.45 ms |  0.901 ms |  1.376 ms | 4333.3333 | 3555.5556 | 1111.1111 | 101.51 MB |
| Sync   | True        |  40.09 ms |  0.476 ms |  0.445 ms | 2000.0000 |  888.8889 |  777.7778 |  80.14 MB |

/cc @MichelZ and @ErikEJ for interest

@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 19, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 3161 in repo dotnet/SqlClient

@mdaigle
Copy link
Contributor

mdaigle commented Feb 19, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mdaigle
Copy link
Contributor

mdaigle commented Feb 19, 2025

Commenter does not have sufficient privileges for PR 3161 in repo dotnet/SqlClient

Unfortunately, new security requirements restrict our ability to run CI for non-maintainers. Feel free to ping the team alias (@dotnet/sqlclientdevteam) or any of us directly and we'll do our best to trigger a pipeline run ASAP.

See also: #3152

@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 23, 2025

Based on the feedback provided I have added a new compatibility switch called Switch.Microsoft.Data.SqlClient.UseCompatibilityAsyncBehaviour which will disable the new behaviour. Is CompatProcessSni enabled the new switch will automatically return true. The new async behaviour is now enabled by default in this PR. Can someone re-run the CI please.

@ErikEJ
Copy link
Contributor

ErikEJ commented Feb 23, 2025

@Wraith2 Is this the "final" PR ?? (I lost track a few years ago)

@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 23, 2025

Yup, part 3 of 3 of #2608 so this build should be faster stable async strings.

@paulmedynski
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@paulmedynski paulmedynski requested a review from a team February 24, 2025 12:51
paulmedynski
paulmedynski requested changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

Some comments for discussion, and a couple of typos.

@cheenamalhotra
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 25, 2025

CI is broken. Can you re-kick it please.

@paulmedynski paulmedynski requested a review from a team February 25, 2025 18:41
@mdaigle
Copy link
Contributor

mdaigle commented Feb 26, 2025

I restarted the failing jobs

@Wraith2
Copy link
Contributor Author

Wraith2 commented Feb 26, 2025

The jobs are failing because the manual tests are timing out. The tests are timing out because stream operations are hitting a path that I wasn't aware of (and didn't break the last two times I reimplemented this) so more work is needed.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Mar 2, 2025

I've made many changes to get streams working. Most things seem to work now but it's hard to tell with how test experience is locally. Can someone on the @dotnet/sqlclientdevteam run the CI please.

@paulmedynski
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@paulmedynski paulmedynski requested a review from a team March 3, 2025 12:09
@codecov Codecov
Copy link

codecov bot commented Mar 3, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 73.17073% with 99 lines in your changes missing coverage. Please review.

Project coverage is 66.15%. Comparing base (1e59b88) to head (671580c).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 59.22% 42 Missing ⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 66.66% 34 Missing ⚠️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 86.95% 18 Missing ⚠️
...icrosoft/Data/SqlClient/LocalAppContextSwitches.cs 71.42% 2 Missing ⚠️
...nt/src/Microsoft/Data/SqlClient/SqlCachedBuffer.cs 88.88% 2 Missing ⚠️
...core/src/Microsoft/Data/SqlClient/SqlDataReader.cs 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (1e59b88) and HEAD (671580c). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (1e59b88) HEAD (671580c)
addons 1 0
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3161      +/-   ##
==========================================
- Coverage   72.58%   66.15%   -6.43%     
==========================================
  Files         289      283       -6     
  Lines       59503    59363     -140     
==========================================
- Hits        43188    39271    -3917     
- Misses      16315    20092    +3777
Flag Coverage Δ
addons ?
netcore 68.92% <78.40%> (-6.20%) ⬇️
netfx 65.12% <75.93%> (-6.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This was referenced Aug 18, 2025
Open
Closed
Merged
Closed
Open
Closed
Closed
Closed
Open
Open
Merged
Closed
Open
Closed
Merged
This was referenced Aug 25, 2025
Closed
Open
Open
Closed
Closed
Closed
Closed
Closed
Closed
Open
Open
Closed
Open
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdaigle mdaigle mdaigle approved these changes

@paulmedynski paulmedynski paulmedynski approved these changes

+1 more reviewer

@edwardneal edwardneal edwardneal left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.1-preview1
Development

Successfully merging this pull request may close these issues.
9 participants
@Wraith2 @mdaigle @ErikEJ @paulmedynski @cheenamalhotra @MichelZ @brendangooden @David-Engel @edwardneal