Skip to content
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

Simplify SNI tracing #1187

Merged
merged 1 commit into from
Jul 28, 2021
Merged

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Jul 25, 2021

In some methods the code overhead of event tracing becomes higher than the code itself. For example:

        public override uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = null)
        {
            long scopeID = SqlClientEventSource.Log.TrySNIScopeEnterEvent(s_className);
            try
            {
                SNIAsyncCallback cb = callback ?? _sendCallback;
                packet.WriteToStreamAsync(_stream, cb, SNIProviders.TCP_PROV);
                SqlClientEventSource.Log.TrySNITraceEvent(s_className, EventType.INFO, "Connection Id {0}, Data sent to stream asynchronously", args0: _connectionId);
                return TdsEnums.SNI_SUCCESS_IO_PENDING;
            }
            finally
            {
                SqlClientEventSource.Log.TrySNIScopeLeaveEvent(scopeID);
            }
        }

Using the TrySNIEventScope struct introduced in the Mars muxer rework this can be simplified to:

        public override uint SendAsync(SNIPacket packet, SNIAsyncCallback callback = null)
        {
            using (TrySNIEventScope.Create(nameof(SNITCPHandle)))
            {
                SNIAsyncCallback cb = callback ?? _sendCallback;
                packet.WriteToStreamAsync(_stream, cb, SNIProviders.TCP_PROV);
                SqlClientEventSource.Log.TrySNITraceEvent(nameof(SNITCPHandle), EventType.INFO, "Connection Id {0}, Data sent to stream asynchronously", args0: _connectionId);
                return TdsEnums.SNI_SUCCESS_IO_PENDING;
            }
        }

which is visually cleaner allowing you to focus on the important code not tracing.

I have also removed use of constant s_className variables because using the nameof() expression allows the compiler to embed and intern the required string making codegen cleaner. Instead of loading a variable each time the literal string location in memory can be hardcoded by the jit reducing the tracing overhead even when it is not used by reducing codesize.

@JRahnama
Copy link
Member

LGTM. Nice approach. Have you tested the results from PerfView as well? Can you share the results to see if those are working as expected please?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jul 28, 2021

I haven't tested from perfview, I'm not sure how I'd do that, I'm not an experienced user of perfview.
Logically the output should be identical.

@JRahnama
Copy link
Member

My only concern was that EventSource is happenign out of the process (this is why it is not good to capture serialized objects) and I was wondering about whent he dispose happens and how/where the log is going to be printed, but according to MS documentation it is fine using that.

Once more Thanks for the support.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jul 28, 2021

Usings are lowered by the compiler into try finally blocks. In the case where there are multiple using blocks they're lowered individually so that inner finally blocks happen before outers so we get preserved ordering. We should end up with equivalent code or in the case with multiple usings slightly safer code (because an error in a finally won't prevent another finally).

The intention and expected result is that everything should function exactly as before but be slightly easier to read.

Copy link
Contributor

@johnnypham johnnypham left a comment

Choose a reason for hiding this comment

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

Event ordering looked identical in my tests. LGTM!

@cheenamalhotra cheenamalhotra added this to In progress in SqlClient v4.0 via automation Jul 28, 2021
@cheenamalhotra cheenamalhotra added this to the 4.0.0-preview1 milestone Jul 28, 2021
@cheenamalhotra cheenamalhotra merged commit 4e72e43 into dotnet:main Jul 28, 2021
SqlClient v4.0 automation moved this from In progress to Done Jul 28, 2021
@Wraith2 Wraith2 deleted the sni-simplifytracing branch July 29, 2021 18:19
@cheenamalhotra cheenamalhotra added the ➕ Code Health Changes related to source code improvements label Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Changes related to source code improvements
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants