Skip to content

Replace ISystemClock with TimeProvider in Kestrel #48081

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

Merged
merged 17 commits into from
May 19, 2023

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented May 4, 2023

Fixes #47472
Fixes #13628

This replaces the internal ISystemClock types with the new TimeProvider abstraction. I also included #13628 which switches most operations to timestamps instead of utc dates, which may clean up the code a bit and allow for other micro-optimizations by working more directly with longs than DateTimeOffset.

Fixes #37862 (quarantined test which hasn't failed in 30+ days)

@Tratcher Tratcher force-pushed the tratcher/clocks branch from bdb1a0f to 7c93c51 Compare May 8, 2023 19:05
@danmoseley
Copy link
Member

danmoseley commented May 11, 2023

will we eventually have only one TestTimeProvider? looks like 4 are copies, plus the SignalR one is possibly mergeable

edit: never mind, you answered that already 😄

@Tratcher
Copy link
Member Author

/benchmarks

@pr-benchmarks
Copy link

pr-benchmarks bot commented May 11, 2023

Crank Pull Request Bot

/benchmark <benchmark[,...]> <profile[,...]> <component,[...]> <arguments>

Benchmarks:

  • plaintext: TechEmpower Plaintext Scenario - ASP.NET Platform implementation
  • json: TechEmpower JSON Scenario - ASP.NET Platform implementation
  • fortunes: TechEmpower Fortunes Scenario - ASP.NET Platform implementation
  • yarp: YARP - http-http with 10 bytes
  • mvcjsoninput2k: Sends 2Kb Json Body to an MVC controller

Profiles:

  • aspnet-perf-lin: INTEL/Linux 12 Cores
  • aspnet-perf-win: INTEL/Windows 12 Cores
  • aspnet-citrine-lin: INTEL/Linux 28 Cores
  • aspnet-citrine-win: INTEL/Windows 28 Cores
  • aspnet-citrine-ampere: Ampere/Linux 80 Cores
  • aspnet-citrine-amd: AMD/Linux 48 Cores

Components:

  • kestrel
  • mvc

Arguments: any additional arguments to pass through to crank, e.g. --variable name=value

@Tratcher
Copy link
Member Author

/benchmark plaintext aspnet-citrine-lin kestrel

@pr-benchmarks
Copy link

pr-benchmarks bot commented May 12, 2023

Benchmark started for plaintext on aspnet-citrine-lin with kestrel. Logs: link

@Tratcher
Copy link
Member Author

/benchmark plaintext aspnet-citrine-lin kestrel

@pr-benchmarks
Copy link

pr-benchmarks bot commented May 15, 2023

Benchmark started for plaintext on aspnet-citrine-lin with kestrel. Logs: link

@Tratcher
Copy link
Member Author

Tratcher commented May 15, 2023

will we eventually have only one TestTimeProvider? looks like 4 are copies, plus the SignalR one is possibly mergeable

I got rid of two custom time providers in this PR, one in product code and one test. Once this review is done I can look at reconciling across AspNetCore.

Update I got rid of all duplicate test time providers in AspNetCore.

@Tratcher
Copy link
Member Author

/benchmark plaintext aspnet-citrine-lin kestrel

@pr-benchmarks
Copy link

pr-benchmarks bot commented May 16, 2023

Benchmark started for plaintext on aspnet-citrine-lin with kestrel. Logs: link

@sebastienros
Copy link
Member

/benchmark plaintext aspnet-citrine-lin kestrel --application.sdkVersion 8.0.100-preview.5.23260.3

@pr-benchmarks
Copy link

pr-benchmarks bot commented May 16, 2023

Benchmark started for plaintext on aspnet-citrine-lin with kestrel and arguments --application.sdkVersion 8.0.100-preview.5.23260.3. Logs: link

@sebastienros
Copy link
Member

| application           | plaintext.base            | plaintext.pr              |        |
| --------------------- | ------------------------- | ------------------------- | ------ |
| CPU Usage (%)         |                        99 |                        99 |  0.00% |
| Cores usage (%)       |                     2,774 |                     2,777 | +0.11% |
| Working Set (MB)      |                       122 |                       121 | -0.82% |
| Private Memory (MB)   |                       655 |                       651 | -0.61% |
| Build Time (ms)       |                     4,778 |                     4,378 | -8.37% |
| Start Time (ms)       |                       210 |                       210 |  0.00% |
| Published Size (KB)   |                    96,262 |                    96,262 |  0.00% |
| Symbols Size (KB)     |                        53 |                        53 |  0.00% |
| .NET Core SDK Version | 8.0.100-preview.5.23260.3 | 8.0.100-preview.5.23260.3 |        |


| load                   | plaintext.base | plaintext.pr |         |
| ---------------------- | -------------- | ------------ | ------- |
| CPU Usage (%)          |             93 |           94 |  +1.08% |
| Cores usage (%)        |          2,613 |        2,642 |  +1.11% |
| Working Set (MB)       |             48 |           48 |   0.00% |
| Private Memory (MB)    |            370 |          370 |   0.00% |
| Start Time (ms)        |              0 |            0 |         |
| First Request (ms)     |            103 |           99 |  -3.88% |
| Requests/sec           |     11,071,054 |   11,138,157 |  +0.61% |
| Requests               |    167,173,536 |  168,100,896 |  +0.55% |
| Mean latency (ms)      |           1.28 |         1.22 |  -4.69% |
| Max latency (ms)       |          61.79 |        91.46 | +48.02% |
| Bad responses          |              0 |            0 |         |
| Socket errors          |              0 |            0 |         |
| Read throughput (MB/s) |       1,331.20 |     1,341.44 |  +0.77% |
| Latency 50th (ms)      |           0.74 |         0.73 |  -1.34% |
| Latency 75th (ms)      |           1.13 |         1.10 |  -2.65% |
| Latency 90th (ms)      |           1.92 |         1.81 |  -5.73% |
| Latency 99th (ms)      |          14.03 |        13.36 |  -4.78% |

@Tratcher
Copy link
Member Author

/benchmark plaintext aspnet-citrine-lin kestrel --application.sdkVersion 8.0.100-preview.5.23260.3

@pr-benchmarks
Copy link

pr-benchmarks bot commented May 16, 2023

Benchmark started for plaintext on aspnet-citrine-lin with kestrel and arguments --application.sdkVersion 8.0.100-preview.5.23260.3. Logs: link

@Tratcher
Copy link
Member Author

| application           | plaintext.base            | plaintext.pr              |        |
| --------------------- | ------------------------- | ------------------------- | ------ |
| CPU Usage (%)         |                       100 |                        99 | -1.00% |
| Cores usage (%)       |                     2,787 |                     2,773 | -0.50% |
| Working Set (MB)      |                       124 |                       120 | -3.23% |
| Private Memory (MB)   |                       656 |                       651 | -0.76% |
| Build Time (ms)       |                     4,749 |                     4,289 | -9.69% |
| Start Time (ms)       |                       208 |                       207 | -0.48% |
| Published Size (KB)   |                    96,262 |                    96,262 |  0.00% |
| Symbols Size (KB)     |                        53 |                        53 |  0.00% |
| .NET Core SDK Version | 8.0.100-preview.5.23260.3 | 8.0.100-preview.5.23260.3 |        |


| load                   | plaintext.base | plaintext.pr |         |
| ---------------------- | -------------- | ------------ | ------- |
| CPU Usage (%)          |             94 |           95 |  +1.06% |
| Cores usage (%)        |          2,627 |        2,664 |  +1.41% |
| Working Set (MB)       |             48 |           48 |   0.00% |
| Private Memory (MB)    |            370 |          370 |   0.00% |
| Start Time (ms)        |              0 |            0 |         |
| First Request (ms)     |             92 |           96 |  +4.35% |
| Requests/sec           |     11,113,351 |   11,232,468 |  +1.07% |
| Requests               |    167,519,747 |  169,255,920 |  +1.04% |
| Mean latency (ms)      |           1.62 |         1.21 | -25.31% |
| Max latency (ms)       |         199.10 |        67.80 | -65.95% |
| Bad responses          |              0 |            0 |         |
| Socket errors          |              0 |            0 |         |
| Read throughput (MB/s) |       1,331.20 |     1,351.68 |  +1.54% |
| Latency 50th (ms)      |           0.74 |         0.73 |  -2.16% |
| Latency 75th (ms)      |           1.12 |         1.09 |  -2.68% |
| Latency 90th (ms)      |           1.88 |         1.74 |  -7.45% |
| Latency 99th (ms)      |          16.75 |        13.51 | -19.34% |

The few metrics that looked concerning are unstable across runs. I think this is good as far as perf goes.

@Tratcher Tratcher marked this pull request as ready for review May 16, 2023 22:38
@Tratcher Tratcher requested a review from mgravell as a code owner May 16, 2023 22:38
@Tratcher Tratcher added this to the 8.0-preview5 milestone May 16, 2023
@Tratcher Tratcher requested a review from sebastienros May 17, 2023 15:53
@Tratcher
Copy link
Member Author

/benchmark plaintext aspnet-citrine-lin kestrel --application.sdkVersion 8.0.100-preview.5.23260.3

@pr-benchmarks
Copy link

pr-benchmarks bot commented May 18, 2023

Benchmark started for plaintext on aspnet-citrine-lin with kestrel and arguments --application.sdkVersion 8.0.100-preview.5.23260.3. Logs: link

@Tratcher Tratcher enabled auto-merge (squash) May 18, 2023 22:58
@Tratcher Tratcher merged commit 7c89649 into dotnet:main May 19, 2023
@Tratcher Tratcher deleted the tratcher/clocks branch May 19, 2023 17:32
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel
Projects
None yet
7 participants