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

Optimizing Eventsource memory allocations and removing an extra comment. #684

Merged
merged 14 commits into from
Aug 21, 2020

Conversation

JRahnama
Copy link
Member

@JRahnama JRahnama commented Aug 11, 2020

This PR is addressing Issue#92 and Issue#681.

  • Issue#92 regarding // call OnError outside of _ErrorCollectionLock to avoid deadlock comment, apparently the lock is moved inside a function called GetFullErrorAndWarningCollection to avoid deadlocks. comment is safely removed.

  • Issue#681: This is a first step of making EventSource optimized. all ToString() methods are removed from eventsource related code implementation, if possible, and moved inside EventSource class inside the if checkers to see if the log is requested, for perf improvement. all arguments passed to NonEvent functions are checked for null values and null-coalescing operator is applied inside EventSource class.

New methods have been added to EventSource class overloads. This methods do not have the IsEnabled checker inside them. They are used when IsEnabled is called out of EventSource class, for instance when a setup was needed to make one or more variables to be passed to EventSource functions. These methods have the bare name such as TraceEvent etc.

Older methods in NonEvent overloads region of EventSource class, the one with IsEnabled checker inside, a prefix of Try is added to the old names to distinguish between two different kind of overloads.

There were places that log levels, such as "INFO' or "API" and etc. as a separate string to the string.Format method, I have combined those two strings to be one and use a different overload from EventSource.
At some places while using null coalescing the log message was passing a string containing "null" or "", in possible cases I have removed them as the null checker is applied inside EventSource class.

@JRahnama JRahnama changed the title Issue 681 and issue 92 Optimizing Eventsource memory allocations and removing an extra comment. Aug 11, 2020
Copy link
Contributor

@TrayanZapryanov TrayanZapryanov left a comment

Choose a reason for hiding this comment

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

I leaved several comments.

@cheenamalhotra cheenamalhotra added this to the 2.1.0-preview1 milestone Aug 13, 2020
@cheenamalhotra cheenamalhotra added the 📈 Performance Use this label for performance improvement activities label Aug 18, 2020
# Conflicts:
#	src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlAuthenticationProviderManager.NetCoreApp.cs
#	src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlAuthenticationProviderManager.cs
@cheenamalhotra cheenamalhotra merged commit 9e23679 into dotnet:master Aug 21, 2020
TrayanZapryanov pushed a commit to TrayanZapryanov/SqlClient that referenced this pull request Aug 31, 2020
TrayanZapryanov added a commit to TrayanZapryanov/SqlClient that referenced this pull request Aug 31, 2020
@JRahnama JRahnama deleted the Issue-92-issue-681 branch July 6, 2021 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📈 Performance Use this label for performance improvement activities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SqlClientEventSource.Log.TraceEvent(...) leads to not necessary allocations in several places
6 participants