Skip to content

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

Merged
cheenamalhotra merged 14 commits into
dotnet:masterfrom
JRahnama:Issue-92-issue-681
Aug 21, 2020
Merged

Optimizing Eventsource memory allocations and removing an extra comment.#684
cheenamalhotra merged 14 commits into
dotnet:masterfrom
JRahnama:Issue-92-issue-681

Conversation

@JRahnama

@JRahnamaJRahnama commented Aug 11, 2020

Copy link
Copy Markdown
Contributor

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.

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

@TrayanZapryanovTrayanZapryanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I leaved several comments.

@cheenamalhotracheenamalhotra added this to the 2.1.0-preview1 milestone Aug 13, 2020
@cheenamalhotracheenamalhotra added the Performance 📈 Issues that are targeted to performance improvements. 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
Comment threadsrc/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlError.cs Outdated
@cheenamalhotra
cheenamalhotra merged commit 9e23679 into dotnet:masterAug 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 freeto 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.

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

6 participants

@JRahnama@TrayanZapryanov@cheenamalhotra@johnnypham@karinazhou@DavoudEshtehari