Cache tags from DbCommand#774
Conversation
|
2 things to take care of before merging:
|
|
I could think of a situation where we would have an infinite number of connection strings: if the user opens arbitrary Excel files with ADO.NET. This is very unlikely but still possible, so I added a fail-safe mechanism. There's no noticeable impact on the benchmark (expected, since the fast path doesn't change). BenchmarkDotNet=v0.12.0, OS=Windows 10.0.19041
Intel Core i7-9750H CPU 2.60GHz, 1 CPU, 12 logical and 6 physical cores
[Host] : .NET Framework 4.8 (4.8.4180.0), X64 RyuJIT
Job-VTGAFA : .NET Framework 4.8 (4.8.4180.0), X64 RyuJIT
Job-MNUBEA : .NET Core 3.1.5 (CoreCLR 4.700.20.26901, CoreFX 4.700.20.27001), X64 RyuJIT
|
lucaspimentel
left a comment
lucaspimentel
left a comment
There was a problem hiding this comment.
LGTM. Left a non-blocking question to satisfy my curiosity.
| { | ||
| // Populating the cache. This path should be hit only during application warmup | ||
| // ReSharper disable once ConvertClosureToMethodGroup -- Lambdas are cached by the compiler | ||
| return cache.GetOrAdd(connectionString, cs => ExtractTagsFromConnectionString(cs)); |
There was a problem hiding this comment.
Just curious, is the "Resharper disable" comment just to avoid writing it like this? If so, is there a performance reason for this, or just cosmetic?
| return cache.GetOrAdd(connectionString, cs => ExtractTagsFromConnectionString(cs)); | |
| return cache.GetOrAdd(connectionString, ExtractTagsFromConnectionString); |
There was a problem hiding this comment.
Yes, the comment is there to prevent Resharper from rewriting into a method group. The delegate is cached when using lambdas but not when using method groups, so that would cause an additional heap allocation at each call.
dotnet/roslyn#5835
3 participants
Parsing the connection string is a costly operation that we do at every query, even though we can reasonably expect users to have a limited set of connection strings.
Benchmark: