Skip to content
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

Add back IComparable-based optimization to FrozenDictionary/Set #84301

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

stephentoub
Copy link
Member

We previously had an optimization in FrozenDictionary/Set that special-cased a small number of value types when the default comparer is used... if that type was IComparable, we would sort the types, which would then a) enable us to immediately reject items larger than the known max, and b) enable us to stop searching once we hit an item larger than the one for which we were searching (which then implicitly also immediately rules out items smaller than the known min).

We removed that optimization in general because some prominent IComparable implementations don't always work, in particular container types like ValueTuple that implement IComparable but then it's only functional if the contained types are also comparable. This commit puts it back for an allow-list of types.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We previously had an optimization in FrozenDictionary/Set that special-cased a small number of value types when the default comparer is used... if that type was IComparable, we would sort the types, which would then a) enable us to immediately reject items larger than the known max, and b) enable us to stop searching once we hit an item larger than the one for which we were searching (which then implicitly also immediately rules out items smaller than the known min).

We removed that optimization in general because some prominent IComparable implementations don't always work, in particular container types like ValueTuple that implement IComparable but then it's only functional if the contained types are also comparable.  This commit puts it back for an allow-list of types.
@ghost
Copy link

ghost commented Apr 4, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

We previously had an optimization in FrozenDictionary/Set that special-cased a small number of value types when the default comparer is used... if that type was IComparable, we would sort the types, which would then a) enable us to immediately reject items larger than the known max, and b) enable us to stop searching once we hit an item larger than the one for which we were searching (which then implicitly also immediately rules out items smaller than the known min).

We removed that optimization in general because some prominent IComparable implementations don't always work, in particular container types like ValueTuple that implement IComparable but then it's only functional if the contained types are also comparable. This commit puts it back for an allow-list of types.

Author: stephentoub
Assignees: -
Labels:

area-System.Collections

Milestone: -

@stephentoub stephentoub merged commit 2d05a60 into dotnet:main Apr 6, 2023
@stephentoub stephentoub deleted the addbackcomparable branch April 6, 2023 02:32
@IDisposable
Copy link
Contributor

Just for information, what determines that an IComparable<T> interface would be considered good enough to use this optimization (two part question):

  1. What would we have to guarantee/do to be considered fast enough
  2. How (in the future) could we mark ourselves as meeting that requirement (e.g. IFastComparable or something)

@stephentoub
Copy link
Member Author

It's a minor detail of this particular internal optimization. There's no opting into it. This is really just special-casing a few known types to enable a specific optimization with them.

@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants