-
Notifications
You must be signed in to change notification settings - Fork 286
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
[.NET Core] Event Source traces revision - Part 3 #897
Conversation
…ntSource # Conflicts: # src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Outdated
Show resolved
Hide resolved
Before releasing this can we make sure we do a memory profile and see if we're causing any new allocations from the tracing? These add to some very hot paths and could have high performance costs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review...
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: David Engel <dengel@magnitude.com>
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: David Engel <dengel@magnitude.com>
Co-authored-by: David Engel <dengel@magnitude.com>
d462365
to
d237e67
Compare
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/LocalDB.Windows.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNIMarsHandle.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a constant string to address a class method instead of using the strong types makes it vulnerable to future maintenance and possible misinformation. I suggest avoiding this approach at least hereafter.
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlClientEventSource.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlClientEventSource.cs
Outdated
Show resolved
Hide resolved
Please checkout latest commit to avoid human errors in method/class names, let me know if you want to propose a change before I go forward to rest of files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the perf report for Managed SNI
on Windows without enabling the traces by Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows
=true
:
- Microsoft.Data.SqlClient V2.1.1:
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i7-8665U CPU 1.90GHz (Coffee Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.103
[Host] : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT
Job-EEOJRM : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT
IterationCount=10 LaunchCount=1 WarmupCount=10
Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated | Completed Work Items | Lock Contentions |
---|---|---|---|---|---|---|---|---|---|
Connection_Open | 8.236 μs | 0.1842 μs | 0.1096 μs | 0.2747 | 0.0916 | - | 1.13 KB | 0.0000 | - |
Connection_OpenAsync | 9.885 μs | 2.5592 μs | 1.6927 μs | 0.3204 | 0.1068 | - | 1.32 KB | 0.0000 | - |
ExecuteScalar | 25,271.486 μs | 2,088.2995 μs | 1,242.7135 μs | 406.2500 | 406.2500 | 406.2500 | 20482.32 KB | 0.0625 | - |
ExecuteScalarAsync | 3,478,298.100 μs | 411,327.5428 μs | 272,067.7018 μs | 3000.0000 | 2000.0000 | 1000.0000 | 31193.54 KB | 1320.0000 | - |
ExecuteNonQuery | 17,598.683 μs | 1,469.9404 μs | 972.2745 μs | - | - | - | 1.81 KB | 0.0625 | - |
ExecuteNonQueryAsync | 18,371.153 μs | 931.5047 μs | 616.1327 μs | - | - | - | 3.44 KB | 3.0625 | - |
ExecuteReader | 29,459.933 μs | 2,726.4595 μs | 1,622.4723 μs | 406.2500 | 406.2500 | 406.2500 | 20482.6 KB | 0.0625 | - |
ExecuteReaderAsync | 3,225,492.210 μs | 485,161.6103 μs | 320,904.3659 μs | 3000.0000 | 2000.0000 | 1000.0000 | 31193.24 KB | 1318.0000 | - |
ExecuteXmlReader | 102,757.710 μs | 3,769.9511 μs | 2,493.5892 μs | 7000.0000 | 4000.0000 | 1800.0000 | 82081.99 KB | 0.4000 | - |
ExecuteXmlReaderAsync | 114,564.100 μs | 6,156.8096 μs | 3,220.1305 μs | 5200.0000 | 2800.0000 | 800.0000 | 82156.11 KB | 3.4000 | - |
- Current change from here:
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19042
Intel Core i7-8665U CPU 1.90GHz (Coffee Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.103
[Host] : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT
Job-FSZQZJ : .NET Core 3.1.12 (CoreCLR 4.700.21.6504, CoreFX 4.700.21.6905), X64 RyuJIT
IterationCount=10 LaunchCount=1 WarmupCount=10
Method | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated | Completed Work Items | Lock Contentions |
---|---|---|---|---|---|---|---|---|---|
Connection_Open | 7.535 μs | 0.5108 μs | 0.2672 μs | 0.2747 | 0.0916 | - | 1.13 KB | 0.0000 | - |
Connection_OpenAsync | 7.358 μs | 0.8022 μs | 0.4196 μs | 0.3204 | 0.1068 | - | 1.32 KB | 0.0000 | - |
ExecuteScalar | 25,644.887 μs | 3,266.8449 μs | 2,160.8157 μs | 406.2500 | 406.2500 | 406.2500 | 20482.88 KB | 0.0625 | - |
ExecuteScalarAsync | 3,110,033.890 μs | 386,661.8388 μs | 255,752.8658 μs | 3000.0000 | 2000.0000 | 1000.0000 | 31194.66 KB | 1320.0000 | - |
ExecuteNonQuery | 14,510.836 μs | 239.7518 μs | 158.5810 μs | - | - | - | 2.42 KB | 0.0625 | - |
ExecuteNonQueryAsync | 15,998.277 μs | 329.1947 μs | 195.8985 μs | - | - | - | 4.28 KB | 3.0625 | - |
ExecuteReader | 30,085.312 μs | 3,241.0842 μs | 1,695.1497 μs | 406.2500 | 406.2500 | 406.2500 | 20483.16 KB | 0.0625 | - |
ExecuteReaderAsync | 2,936,545.167 μs | 304,153.8034 μs | 180,997.0497 μs | 3000.0000 | 2000.0000 | 1000.0000 | 31194.09 KB | 1318.0000 | - |
ExecuteXmlReader | 128,351.173 μs | 19,679.6004 μs | 13,016.8372 μs | 7000.0000 | 4000.0000 | 1750.0000 | 82082.99 KB | 0.5000 | - |
ExecuteXmlReaderAsync | 135,127.420 μs | 42,969.2720 μs | 28,421.5130 μs | 5500.0000 | 3250.0000 | 1000.0000 | 82158.5 KB | 3.5000 | - |
LGTM |
56270cd
to
c89c7e0
Compare
c89c7e0
to
de8fc98
Compare
Managed SNI layer full tracing coverage from TdsParser.