Conversation
There was a problem hiding this comment.
Pull request overview
Refactors several internal dictionary access patterns to avoid redundant ContainsKey + indexer lookups, primarily by switching to TryGetValue (and, for the DNS cache, direct indexer assignment) while preserving existing behavior in SqlDependency-related infrastructure.
Changes:
- Replace double-lookups with
TryGetValueinSqlDependency*code paths. - Simplify DNS cache insertion to a single overwrite assignment.
- Update server enumerator parsing to use
TryGetValuefor instance detail extraction.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDependencyUtils.cs | Uses TryGetValue for dependency ID lookup under lock. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDependencyListener.cs | Replaces repeated dictionary lookups with TryGetValue in app-domain/container tracking paths. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDependency.cs | Uses TryGetValue to avoid multiple lookups in server/user hash management and default options composition. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SQLFallbackDNSCache.cs | Simplifies add/replace semantics to a single assignment into the ConcurrentDictionary. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/Sql/SqlDataSourceEnumeratorManagedHelper.netcore.cs | Uses TryGetValue when populating DataRow fields from parsed instance details. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
A glorious set of changes! 🚀 Just a couple of comments.
| _appDomainKeyHash[appDomainKey] = _appDomainKeyHash[appDomainKey] + 1; | ||
| SqlClientEventSource.Log.TryNotificationTraceEvent("SqlConnectionContainer.IncrementStartCount|DEP> _appDomainKeyHash contained AppDomainKey: '{0}', incremented count: '{1}'.", appDomainKey, _appDomainKeyHash[appDomainKey]); | ||
| count++; | ||
| _appDomainKeyHash[appDomainKey] = count; |
There was a problem hiding this comment.
You can eliminate the increment here:
_appDomainKeyHash[appDomainKey] = count + 1;
There was a problem hiding this comment.
that would result in the incorrect count being logged below
| if (_appDomainKeyHash.ContainsKey(appDomainKey)) | ||
| if (_appDomainKeyHash.TryGetValue(appDomainKey, out int count)) | ||
| { | ||
| _appDomainKeyHash[appDomainKey] = _appDomainKeyHash[appDomainKey] + 1; |
There was a problem hiding this comment.
Wow 3 identical lookups under the same lock 😄
| if (!_sqlDependencyPerAppDomainDispatchers.ContainsKey(appDomainKey)) | ||
| { | ||
| _sqlDependencyPerAppDomainDispatchers[appDomainKey] = dispatcher; | ||
| _sqlDependencyPerAppDomainDispatchers.Add(appDomainKey, dispatcher); |
There was a problem hiding this comment.
I think TryAdd() would work here and eliminate the explicit check above.
There was a problem hiding this comment.
TryAdd doesnt exist in net classic
github-project-automation
bot
moved this from To triage
to In progress
in SqlClient Board
No description provided.