Skip to content

Conversation

@SAY14489
Copy link
Contributor

@SAY14489 SAY14489 commented Sep 11, 2025

Description

This change optimizes the performance of SQL execution timing by replacing expensive DateTime operations with lightweight Environment.TickCount calls in SqlStatistics. The optimization targets the RequestExecutionTimer() and ReleaseAndUpdateExecutionTimer() methods, which are frequently called during database operations. This issue was exposed by profilers which showed that these two calling methods made up a significant portion of DateTime.UtcNow CPU usage.

Performance improvement:

Method Mean Error StdDev Ratio RatioSD Allocated Alloc Ratio
DateTimeUtcNowToFileTimeUtc 57.243 ns 0.9307 ns 0.8250 ns 1.00 0.02 - NA
EnvironmentTickCount 1.272 ns 0.0533 ns 0.1328 ns 0.02 0.00 - NA

^ Done Through BenchmarkDotNet
Benchmarking shows an approximately 55x performance improvement (from ~57ns to ~1ns per timing operation) while maintaining adequate accuracy for database operation monitoring.

Changes made:

  • Added ADP.FastTimerCurrent() method using Environment.TickCount
  • Modified SqlStatistics execution timing methods to use the optimized timer
  • Implemented wraparound handling for 32-bit integer overflow
  • Updated timing units to maintain consistency with existing millisecond reporting

Backwards compatibility:

Fully maintained. All existing APIs, return values, and units remain unchanged. The optimization is internal implementation only.

Scope:

This change specifically optimizes execution timing measurement without affecting timeout logic or other timing-dependent functionality in the codebase.

Issues

Performance optimization. Addresses inefficient use of DateTime.UtcNow.ToFileTimeUtc() for high-frequency elapsed time measurements.

Testing

Manual testing performed:

  • Created test application measuring timing accuracy across different durations (100ms to 2000ms)
  • Verified timing accuracy using Stopwatch as baseline for actual elapsed time measurement and compared against DateTime.UtcNow.ToFileTimeUtc()
  • Confirmed proper accumulation of execution times across multiple measurement periods

Accuracy results:

Testing showed the TickCount optimization maintains acceptable accuracy for duration timing, with error rates under 3% compared to StopWatch baseline for operations lasting 100ms or longer. It is a slight loss in accuracy compared to DateTime.UtcNow.ToFileTimeUtc(), which I believe is due to the lower precision, but for the use case and performance improvement of 55x, it seems like a worthwhile switch.

@SAY14489 SAY14489 requested a review from a team as a code owner September 11, 2025 06:02
Copilot AI review requested due to automatic review settings September 11, 2025 06:02
Copy link
Contributor

Copilot AI left a comment

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 performance by replacing expensive DateTime.UtcNow operations with lightweight Environment.TickCount calls in SqlStatistics. The change provides approximately 55x performance improvement (from ~57ns to ~1ns per timing operation) while maintaining adequate accuracy for database operation monitoring.

Key changes:

  • Added ADP.FastTimerCurrent() method using Environment.TickCount for high-frequency timing operations
  • Modified SqlStatistics execution timing to use nullable timestamp and wraparound handling for 32-bit integer overflow
  • Updated timing units in dictionary output to maintain consistency with existing millisecond reporting

Reviewed Changes

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

File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlStatistics.cs Modified execution timing implementation to use nullable timestamps, added wraparound logic, and updated dictionary output format
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Added FastTimerCurrent() method using Environment.TickCount for optimized timing operations

@SAY14489 SAY14489 changed the title Perf/optimize timer current Optimization: Use Environment.TickCount for SqlStatistics execution timing Sep 11, 2025
@paulmedynski
Copy link
Contributor

/azp run

@paulmedynski paulmedynski self-assigned this Sep 11, 2025
@paulmedynski paulmedynski added this to the 7.0-preview2 milestone Sep 11, 2025
@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@paulmedynski paulmedynski added the Performance 📈 Issues that are targeted to performance improvements. label Sep 11, 2025
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.

Questions about the wrapping algorithms and unit tests.

@SAY14489
Copy link
Contributor Author

@SAY14489 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree company="Microsoft"

@azure-pipelines
Copy link

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

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.

Looks great!

@paulmedynski
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@paulmedynski
Copy link
Contributor

@SAY14489 - Thank you for the great PR, especially your first contribution. Performance improvements like this that are isolated and easy to verify are always welcome!

Copy link
Contributor

@samsharma2700 samsharma2700 left a comment

Choose a reason for hiding this comment

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

Great work!

@SAY14489
Copy link
Contributor Author

Great work!

I’m trying to complete this PR. I just synced this branch with the up to date main since I couldn’t see the merge option, could you please start a pipeline run to fulfill the requirements? Thank You!

@paulmedynski
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.50%. Comparing base (fd16c6d) to head (6058ca0).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ient/src/Microsoft/Data/SqlClient/SqlStatistics.cs 77.77% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (fd16c6d) and HEAD (6058ca0). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (fd16c6d) HEAD (6058ca0)
netfx 2 1
netcore 2 1
addons 2 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3609      +/-   ##
==========================================
- Coverage   70.67%   62.50%   -8.18%     
==========================================
  Files         277      271       -6     
  Lines       61686    61373     -313     
==========================================
- Hits        43598    38360    -5238     
- Misses      18088    23013    +4925     
Flag Coverage Δ
addons ?
netcore 67.67% <81.81%> (-5.70%) ⬇️
netfx 60.86% <81.81%> (-8.72%) ⬇️

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 merged commit 1b1ddec into dotnet:main Sep 16, 2025
236 checks passed
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants