-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Replace a couple of Hashtables in CoreLib with ConcurrentDictionary #122918
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
Conversation
Avoids boxing keys/values, makes things more strongly typed, and reduces some ceremony.
|
Tagging subscribers to this area: @dotnet/area-system-globalization |
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.
Pull request overview
This PR modernizes two classes in CoreLib by replacing legacy Hashtable collections with strongly-typed ConcurrentDictionary<TKey, TValue>, eliminating boxing/unboxing overhead and reducing code ceremony.
Key Changes
- Replaced
Hashtable.Synchronized()withConcurrentDictionaryfor type safety and better performance - Simplified cache lookup patterns by using
GetOrAdd()method instead of manual check-add-lock logic - Adopted modern C# syntax (collection expressions and expression-bodied members)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs | Replaced Hashtable with ConcurrentDictionary<string, int> for encoding name-to-codepage mapping; simplified lookup logic using GetOrAdd |
| src/libraries/System.Private.CoreLib/src/System/CurrentSystemTimeZone.cs | Replaced Hashtable with ConcurrentDictionary<int, DaylightTime> for year-to-daylight-changes caching; eliminated manual double-check locking pattern |
src/libraries/System.Private.CoreLib/src/System/Text/EncodingTable.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/CurrentSystemTimeZone.cs
Outdated
Show resolved
Hide resolved
tarekgh
left a comment
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.
Modulo copilot comments, LGTM!
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Avoids boxing keys/values, makes things more strongly typed, and reduces some ceremony.