Skip to content

[5.1] Optimization: Use Environment.TickCount for SqlStatistics execution timing#3831

Merged
paulmedynski merged 7 commits into
release/5.1from
dev/cheena/5.1-port-sqlstatisticsperf
Dec 15, 2025
Merged

[5.1] Optimization: Use Environment.TickCount for SqlStatistics execution timing#3831
paulmedynski merged 7 commits into
release/5.1from
dev/cheena/5.1-port-sqlstatisticsperf

Conversation

@cheenamalhotra

Copy link
Copy Markdown
Member

Ports #3609 to release/5.1 branch

Closes #3763

Copilot AI review requested due to automatic review settings December 5, 2025 20:18
@cheenamalhotra
cheenamalhotra requested a review from a team as a code owner December 5, 2025 20:18
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Dec 5, 2025
@cheenamalhotra cheenamalhotra moved this from To triage to In review in SqlClient Board Dec 5, 2025
@cheenamalhotra cheenamalhotra added this to the 5.1.9 milestone Dec 5, 2025
@cheenamalhotra cheenamalhotra linked an issue Dec 5, 2025 that may be closed by this pull request

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes SQL execution timing statistics by replacing the DateTime-based timer with Environment.TickCount for measuring ExecutionTime. This change reduces overhead while maintaining accuracy for performance statistics. The PR ports optimization work from #3609 to the release/5.1 branch.

Key changes:

  • Replaced DateTime.UtcNow.ToFileTimeUtc() with Environment.TickCount for ExecutionTime measurements
  • Added wraparound-safe elapsed time calculation to handle TickCount rolling over every ~24.9 days
  • Removed unreliable timing comparison assertions that could fail due to timing precision differences

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
AdapterUtil.cs Added FastTimerCurrent() to return Environment.TickCount and CalculateTickCountElapsed() to calculate elapsed time with wraparound handling
SqlStatistics.cs Changed _startExecutionTimestamp to nullable long, updated ExecutionTime accumulation to use TickCount directly (already in milliseconds), removed TimerToMilliseconds conversion
TickCountElapsedTest.cs New test file validating wraparound handling for TickCount elapsed time calculations
Microsoft.Data.SqlClient.Tests.csproj Registered the new TickCountElapsedTest.cs test file
CopyAllFromReader.cs Removed assertions comparing ConnectionTime/ExecutionTime/NetworkServerTime as these are no longer reliable with different timing mechanisms

Comment thread src/Microsoft.Data.SqlClient/tests/FunctionalTests/TickCountElapsedTest.cs Outdated
…apsedTest.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings December 5, 2025 20:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings December 5, 2025 21:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread src/Microsoft.Data.SqlClient/tests/FunctionalTests/TickCountElapsedTest.cs Outdated
@codecov

codecov Bot commented Dec 6, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.75%. Comparing base (cc5c81a) to head (896a5a0).
⚠️ Report is 1 commits behind head on release/5.1.

Files with missing lines Patch % Lines
...ient/src/Microsoft/Data/SqlClient/SqlStatistics.cs 77.77% 2 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           release/5.1    #3831      +/-   ##
===============================================
- Coverage        71.51%   70.75%   -0.77%     
===============================================
  Files              293      293              
  Lines            61928    61931       +3     
===============================================
- Hits             44289    43817     -472     
- Misses           17639    18114     +475     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 75.00% <81.81%> (-0.03%) ⬇️
netfx 68.75% <81.81%> (-1.16%) ⬇️

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.

@paulmedynski paulmedynski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with Copilot.

@github-project-automation github-project-automation Bot moved this from In review to In progress in SqlClient Board Dec 8, 2025
@paulmedynski paulmedynski self-assigned this Dec 8, 2025
@priyankatiwari08

Copy link
Copy Markdown
Contributor

This branch needs a merge with main to include the codeql.yml file required for CodeQL scanning. The file should be placed under .github/workflows, which is currently missing in this branch. I had a similar experience for one of my PR where merge was blocked with similar code scanning comment, adding the file enabled successful code scanning.

Copilot AI review requested due to automatic review settings December 10, 2025 17:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comment thread src/Microsoft.Data.SqlClient/tests/FunctionalTests/TickCountElapsedTest.cs Outdated
Comment thread src/Microsoft.Data.SqlClient/tests/FunctionalTests/TickCountElapsedTest.cs Outdated
@cheenamalhotra
cheenamalhotra force-pushed the dev/cheena/5.1-port-sqlstatisticsperf branch from 90abd9d to 86fba26 Compare December 11, 2025 18:24
paulmedynski
paulmedynski previously approved these changes Dec 11, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread src/Microsoft.Data.SqlClient/tests/FunctionalTests/TickCountElapsedTest.cs Outdated
@paulmedynski paulmedynski moved this from In progress to In review in SqlClient Board Dec 15, 2025
@paulmedynski
paulmedynski merged commit 69a924e into release/5.1 Dec 15, 2025
76 checks passed
@paulmedynski
paulmedynski deleted the dev/cheena/5.1-port-sqlstatisticsperf branch December 15, 2025 18:34
@github-project-automation github-project-automation Bot moved this from In review to Done in SqlClient Board Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Backport SqlStatistics perf improvement to MDS v5.1

6 participants