-
-
Notifications
You must be signed in to change notification settings - Fork 5
Add early returns for HashSet<T> and OrderedSet<T> polyfills
#51
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
Co-authored-by: Tyrrrz <[email protected]>
HashSet<T> and OrderedSet<T> polyfills
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 adds early Contains checks to TryGetValue polyfill methods for HashSet<T> and SortedSet<T>, aiming to optimize performance for the not-found case by returning early without iterating through the collection.
Key changes:
- Added early
Containscheck inHashSet<T>.TryGetValuebefore iterating - Added early
Containscheck inSortedSet<T>.TryGetValuebefore iterating
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| PolyShim/NetCore20/HashSet.cs | Adds early return optimization using Contains check before iteration |
| PolyShim/NetCore20/SortedSet.cs | Adds early return optimization using Contains check before iteration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!set.Contains(equalValue)) | ||
| { | ||
| actualValue = default; | ||
| return false; | ||
| } | ||
|
|
Copilot
AI
Nov 27, 2025
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.
The early Contains check creates redundant work when the item exists in the set. When the item is found, the code performs:
Contains()check - O(1) lookup- Full iteration through the set - O(n)
This means for the success case (item exists), we're doing O(1) + O(n) work instead of just O(n). The early return only optimizes the failure case (item not found), but adds overhead to the success case.
Consider whether the failure case is common enough to justify degrading the success case performance. If both cases are important, a more efficient implementation would directly use the underlying hash table structure to get the actual value in O(1) time, avoiding the iteration entirely.
| if (!set.Contains(equalValue)) | |
| { | |
| actualValue = default; | |
| return false; | |
| } |
| if (!set.Contains(equalValue)) | ||
| { | ||
| actualValue = default; | ||
| return false; | ||
| } | ||
|
|
Copilot
AI
Nov 27, 2025
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.
The early Contains check creates redundant work when the item exists in the set. When the item is found, the code performs:
Contains()check - O(log n) lookup- Full iteration through the set - O(n)
This means for the success case (item exists), we're doing O(log n) + O(n) work instead of just O(n). The early return only optimizes the failure case (item not found), but adds overhead to the success case.
Consider whether the failure case is common enough to justify degrading the success case performance. If both cases are important, a more efficient implementation would use SortedSet's internal structure more directly to avoid the full iteration.
| if (!set.Contains(equalValue)) | |
| { | |
| actualValue = default; | |
| return false; | |
| } |
Containscheck toHashSet.TryGetValue(O(1) for not-found case)Containscheck toSortedSet.TryGetValue(O(log n) for not-found case)💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.