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

[TrayanZapryanov]

There are several places where calls to SqlClientEventSource.Log.TraceEvent() are allocating no matter that EventSource is not enabled.

• The biggest allocation that I've identified is in SqlConnectionHelper.cs/ConnectionString_Set
  string cstr = ((null != connectionOptions) ? connectionOptions.UsersConnectionStringForTrace() : ""); SqlClientEventSource.Log.TraceEvent("<prov.DbConnectionHelper.ConnectionString_Set|API> {0}, '{1}'", ObjectID, cstr);

A simple benchmark for creating an SqlConnection :
`

[MemoryDiagnoser]
public class Benchmarks
{
[Benchmark]
public SqlConnection Create()
{
return new SqlConnection("Server=myServerAddress;Database=myDataBase;User Id=myUsername;Password=myPassword;");
}
}`

produces following results:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.388 (2004/?/20H1)
Intel Core i7-4770 CPU 3.40GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.1.302
[Host] : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT
DefaultJob : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT
Before:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
Create	488.8 ns	9.64 ns	10.32 ns	0.1507	-	-	632 B

After:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
Create	323.3 ns	2.22 ns	2.96 ns	0.0572	-	-	240 B

• Other places are several .ToStrings() and .ToLongTimeString()

[JRahnama]

@TrayanZapryanov Thanks for bringing this up to our attention. The memory allocation goes to variable cstr, the eventsource calss itself checks if trace is enabled. For perf improvement the entire clause

string cstr = ((null != connectionOptions) ? connectionOptions.UsersConnectionStringForTrace() : "");
SqlClientEventSource.Log.TraceEvent("<prov.DbConnectionHelper.ConnectionString_Set|API> {0}, '{1}'", ObjectID, cstr);

will be moved inside an if statement.

I will look up for the rest,

[TrayanZapryanov]

I've continued my experiment and benchmarked also against "System.Data.SqlClient" nuget(4.8.1).
And results show that maybe there are more things to optimize.
Both performance and allocations are worst.

Benchmark:

[MemoryDiagnoser]
public class Benchmarks
{

	[Benchmark]
	public Microsoft.Data.SqlClient.SqlConnection Create()
	{
		return new Microsoft.Data.SqlClient.SqlConnection("Server=myServerAddress;Database=myDataBase;User Id=myUsername;Password=myPassword;");
	}

	[Benchmark]
	public System.Data.SqlClient.SqlConnection Create_System()
	{
		return new System.Data.SqlClient.SqlConnection("Server=myServerAddress;Database=myDataBase;User Id=myUsername;Password=myPassword;");
	}


	[Benchmark]
	public Microsoft.Data.SqlClient.SqlConnection CreateAzure()
	{
		return new Microsoft.Data.SqlClient.SqlConnection("Server=tcp:myserver.database.windows.net,1433;Database=myDataBase;User ID=mylogin@myserver;Password=myPassword;Trusted_Connection=False;Encrypt=True;");
	}

	[Benchmark]
    public System.Data.SqlClient.SqlConnection CreateAzure_System()
    {
        return new System.Data.SqlClient.SqlConnection("Server=tcp:myserver.database.windows.net,1433;Database=myDataBase;User ID=mylogin@myserver;Password=myPassword;Trusted_Connection=False;Encrypt=True;");
    }
}

and results:
BenchmarkDotNet=v0.12.1, OS=Windows 10.0.19041.388 (2004/?/20H1)
Intel Core i7-4770 CPU 3.40GHz (Haswell), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.1.302
[Host] : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT
DefaultJob : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
Create	460.9 ns	2.77 ns	2.46 ns	0.1507	-	-	632 B
Create_System	216.2 ns	1.69 ns	1.58 ns	0.0629	-	-	264 B
CreateAzure_System	271.3 ns	2.55 ns	2.26 ns	0.0629	-	-	264 B
CreateAzure	652.1 ns	4.36 ns	4.08 ns	0.2556	-	-	1072 B

[Wraith2]

Running Create and Create_System under a memory profile it looks to me as If the difference is down to stringbuilder tostring calls in DbConnectionOptions.ReplacePasswordPwd I think i'd be tempted to see if those allocations can be avoided until they're needed. The whole parsing of the connectionstring itself is also worth looking at to see if Memory<T> can be used instead of creating strings every time.

[TrayanZapryanov]

I've also wonder as we have cache by connection string for connections, is it worth to use similar cache for parsed key/value pairs? This should be ultimate solution, as normal case is to use two or three connection strings per application. And we can save whole parsing...

[Wraith2]

Caching is tricky and likely to hit locking issues, the parameter names are all constants but unless we can demonstrate clear benefits to keeping a cache and a good eviction policy I doubt we'll be able to do much good with it.
If we can limit the amount of intermediate allocations from connection string parsing it'll be a reasonable start imo.

[TrayanZapryanov]

Maybe a simple LRU cache with ten entries can handle 90% of standard usages in one app. Of course reducing allocations will help too, but should not be the main focus. ConnectionPool key can be used as identifier.

[Wraith2]

Sure but the LRU has to be shared which will require locking and when you're moving fast those locks add up. The alternative is to use lock free structures which either require careful implementation and auditing or use of platform provided versions (does the library already reference immutable? if so that's a freebie) Give it a try I suppose and see if you can get any benefit.