-
Notifications
You must be signed in to change notification settings - Fork 135
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
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL. Thanks!
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: