Skip to content

Optimize ImmutableHashSet<T>.IsProperSubsetOf to avoid unnecessary allocations #127368

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.

Sign up for GitHub

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

Draft
aw0lid wants to merge 1 commit into
base: main
Choose a base branch
from
+70 −27
Draft

Optimize ImmutableHashSet<T>.IsProperSubsetOf to avoid unnecessary allocations #127368

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 70 additions & 27 deletions ...aries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableHashSet_1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,20 @@ public bool SetEquals(IEnumerable<T> other)
public bool IsProperSubsetOf(IEnumerable<T> other)
{
Requires.NotNull(other, nameof(other));
if (this.Origin.Root.IsEmpty)
{
return other.Any();
}

if (other is ICollection<T> otherAsICollectionGeneric)
{
return IsProperSubsetOfFastPath(otherAsICollectionGeneric, this.Origin);
}

else if (other is ICollection otherAsICollection && otherAsICollection.Count <= this.Origin.Count)
Copy link
Copy Markdown
Member

@MihaZupan MihaZupan Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it common for types to implement ICollection but not ICollection<T> if they're already implementing IEnumerable<T>?

Copy link
Copy Markdown
Author

@aw0lid aw0lid Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, it’s not very common, but these types still exist and are in use. Since we can easily capture the Count property from them to trigger an early exit, I thought: why not include them? It broadens the optimization's reach with very little overhead

{
return false;
}

return IsProperSubsetOf(other, this.Origin);
}
Expand Down Expand Up @@ -863,49 +877,78 @@ private static MutationResult SymmetricExcept(IEnumerable<T> other, MutationInpu
/// <summary>
/// Performs the set operation on a given data structure.
/// </summary>
private static bool IsProperSubsetOf(IEnumerable<T> other, MutationInput origin)
private static bool IsProperSubsetOfFastPath(ICollection<T> other, MutationInput origin)
{
Requires.NotNull(other, nameof(other));

if (origin.Root.IsEmpty)
{
return other.Any();
}

// To determine whether everything we have is also in another sequence,
// we enumerate the sequence and "tag" whether it's in this collection,
// then consider whether every element in this collection was tagged.
// Since this collection is immutable we cannot directly tag. So instead
// we simply count how many "hits" we have and ensure it's equal to the
// size of this collection. Of course for this to work we need to ensure
// the uniqueness of items in the given sequence, so we create a set based
// on the sequence first.
var otherSet = new HashSet<T>(other, origin.EqualityComparer);
if (origin.Count >= otherSet.Count)
if (other.Count <= origin.Count)
{
return false;
}

int matches = 0;
bool extraFound = false;
foreach (T item in otherSet)
if (other is ImmutableHashSet<T> otherAsImmutableHashSet)
{
if (Contains(item, origin))
{
matches++;
}
else
if (otherAsImmutableHashSet.KeyComparer == origin.EqualityComparer)
{
extraFound = true;
using (var e = new ImmutableHashSet<T>.Enumerator(origin.Root))
{
while (e.MoveNext())
{
if (!otherAsImmutableHashSet.Contains(e.Current))
{
return false;
}
}
}
Comment on lines +893 to +902
Copy link
Copy Markdown
Member

@MihaZupan MihaZupan Apr 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
using (var e = new ImmutableHashSet<T>.Enumerator(origin.Root))
{
while (e.MoveNext())
{
if (!otherAsImmutableHashSet.Contains(e.Current))
{
return false;
}
}
}
foreach (T value in origin.Root)
{
if (!otherAsImmutableHashSet.Contains(value))
{
return false;
}
}

Any reason why all of the loops are manually deconstructed instead of using foreach?

Copy link
Copy Markdown
Author

@aw0lid aw0lid Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the existing implementation, the code iterates over other and queries the ImmutableHashSet, which costs $O(logN)$ per lookup. By manually instantiating the enumerator from the root, I’ve reversed this logic to iterate over the ImmutableHashSet and query the HashSet<T> instead. This shifts the lookup cost to $O(1)$ reducing the overall complexity from $O(NlogN)$ to $O(N)$.

Also, origin (the MutationInput) does not implement IEnumerable<T>, so a direct foreach is not possible. I had to deconstruct it this way to access the underlying nodes while maintaining the performance gains.

That said, I am waiting for my other PR (#126309) to be merged so I can leverage its logic here and further refine this implementation like following this:

private static bool IsProperSubsetOf(IEnumerable<T> other, MutationInput origin)
{
    Requires.NotNull(other, nameof(other));

    if (origin.Root.IsEmpty)
    {
        return other.Any();
    }

    if (other is ICollection<T> otherAsICollectionGeneric)
    {
        // We check for < instead of != because other is not guaranteed to be a set, it could be a collection with duplicates.
        if (otherAsICollectionGeneric.Count <= origin.Count)
        {
            return false;
        }

        switch (other)
        {
            case ImmutableHashSet<T> otherAsImmutableHashSet:
                if (origin.EqualityComparer.Equals(otherAsImmutableHashSet.KeyComparer))
                {
                    return SetEqualsWithImmutableHashset(otherAsImmutableHashSet, origin);
                }
                break;

            case HashSet<T> otherAsHashset:
                if (origin.EqualityComparer.Equals(otherAsHashset.Comparer))
                {
                    return SetEqualsWithHashset(otherAsHashset, origin);
                }
                break;
        }
    }
    else if (other is ICollection otherAsICollection && otherAsICollection.Count <= origin.Count)
    {
        return false;
    }

    var otherSet = new HashSet<T>(other, origin.EqualityComparer);
    if (otherSet.Count <= origin.Count)
    {
        return false;
    }

    return SetEqualsWithHashset(otherSet, origin);
}

return true;
}
}

if (matches == origin.Count && extraFound)
else if (other is HashSet<T> otherAsHashSet)
{
if (otherAsHashSet.Comparer == origin.EqualityComparer)
{
using (var e = new ImmutableHashSet<T>.Enumerator(origin.Root))
{
while (e.MoveNext())
{
if (!otherAsHashSet.Contains(e.Current))
{
return false;
}
}
}
return true;
}
}

return false;
return IsProperSubsetOf(other, origin);
}

/// <summary>
/// Performs the set operation on a given data structure.
/// </summary>
private static bool IsProperSubsetOf(IEnumerable<T> other, MutationInput origin)
{
Requires.NotNull(other, nameof(other));

var otherSet = new HashSet<T>(other, origin.EqualityComparer);
if (otherSet.Count <= origin.Count)
{
return false;
}

using (var e = new ImmutableHashSet<T>.Enumerator(origin.Root))
{
while (e.MoveNext())
{
if (!otherSet.Contains(e.Current))
{
return false;
}
}
}
return true;
}

/// <summary>
Expand Down
Loading