Uh oh!
There was an error while loading. Please reload this page.
Optimize ImmutableHashSet<T>.IsProperSubsetOf to avoid unnecessary allocations - #127368
Optimize ImmutableHashSet<T>.IsProperSubsetOf to avoid unnecessary allocations#127368aw0lid wants to merge 5 commits into
Conversation
Tagging subscribers to this area: @dotnet/area-system-collections |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
e482a29 to
dc4b4b2CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
dc4b4b2 to
e432dfbCompareaw0lid
commented
Jun 22, 2026
Hi everyone, just a gentle follow-up on this PR |
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "d5da4276084a2615ce50353e6cc19ec7e3ea30c6",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "b5d84ecbf4d92d63329e63025099a4a063a65285",
"last_reviewed_commit": "d5da4276084a2615ce50353e6cc19ec7e3ea30c6",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "b5d84ecbf4d92d63329e63025099a4a063a65285",
"last_recorded_worker_run_id": "29725956930",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "e76ce57566c99c85d2e6274e76509f57f9ee61d2",
"review_id": 4730525611
},
{
"commit": "d5da4276084a2615ce50353e6cc19ec7e3ea30c6",
"review_id": 4733113020
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: ImmutableHashSet<T>.IsProperSubsetOf always materialized a new HashSet<T> from other, incurring an O(n) allocation even in cases that could be resolved in O(1) (e.g. other has fewer or equal elements) or without allocation (e.g. other is already an ImmutableHashSet<T>/HashSet<T> with a compatible comparer). This adds avoidable GC pressure for large sets. This PR is part of #127279.
Approach: The public IsProperSubsetOf now short-circuits the empty-origin case (return other.Any()) up front. The private IsProperSubsetOf(other, origin) gains a switch (other) that mirrors the existing SetEquals fast-path structure: for ImmutableHashSet<T>, HashSet<T>, and ICollection<T> it performs an O(1) count check (Count <= origin.Count => false, correct because a proper subset requires the superset to be strictly larger, and this holds even for collections with duplicates since dedup only shrinks the count). When the comparer matches, it reuses SetEqualsWithImmutableHashset/SetEqualsWithHashset to verify containment without allocating. Only the general IEnumerable fallback still allocates a HashSet<T>. Tests cover mismatched comparers, duplicate ICollection elements, and empty-set scenarios.
Summary: The refactor is correct and consistent with the established SetEquals fast-path pattern in this file. The count checks are logically sound: because origin is a set, a proper-subset relationship requires other's distinct count to strictly exceed origin.Count, and Count <= origin.Count on the raw collection safely rejects (dedup can only reduce the count). The comparer guard via EqualityComparer<IEqualityComparer<T>>.Default.Equals correctly restricts the zero-alloc containment path to compatible comparers; mismatched-comparer cases fall through to the allocating new HashSet<T>(other, origin.EqualityComparer) path, preserving prior semantics. The empty-origin handling moved to the public entry point is equivalent, and the private method's fallback still behaves correctly for empty origin when reached via the Builder (Count <= 0 count checks and the subsequent containment/count logic). I found no correctness regressions. The only issue is a cosmetic indentation glitch on the switch (other) line (flagged inline), which formatting validation may reject. Verdict: LGTM once the formatting nit is addressed.
Detailed Findings
No functional issues found. One cosmetic finding is noted inline: the switch (other) statement at line 940 is mis-indented; run dotnet format to align it with the enclosing block so the C# formatting check passes.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 94.9 AIC · ⌖ 10.6 AIC · ⊞ 10K
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Holistic Review
Motivation: ImmutableHashSet<T>.IsProperSubsetOf always materialized a new HashSet<T> from other, incurring an O(n) allocation even in cases resolvable in O(1) (e.g. other has fewer or equal elements) or without allocation (e.g. other is already an ImmutableHashSet<T>/HashSet<T> with a compatible comparer). This adds avoidable GC pressure for large sets. This PR is part of #127279.
Approach: The public IsProperSubsetOf short-circuits the empty-origin case (return other.Any()) up front. The private IsProperSubsetOf(other, origin) gains a switch (other) mirroring the existing SetEquals fast-path structure: for ImmutableHashSet<T>, HashSet<T>, and ICollection<T> it performs an O(1) count check (Count <= origin.Count => false, correct because a proper subset requires the superset to be strictly larger, and this holds even for collections with duplicates since dedup only shrinks the count). When the comparer matches, it reuses SetEqualsWithImmutableHashset/SetEqualsWithHashset to verify containment without allocating. Only the general IEnumerable fallback still allocates a HashSet<T>. Tests cover mismatched comparers, duplicate ICollection elements, and empty-set scenarios.
Summary: The latest commit makes a single change: it corrects the mis-indentation of the switch (other) statement that was flagged in the prior review. It is now aligned (12 spaces) with the enclosing block, so the C# formatting check should pass. No functional code changed in this increment, and the cumulative assessment is unchanged: the refactor is correct, consistent with the established SetEquals fast-path pattern, and introduces no correctness regressions. The count checks are sound, the comparer guard correctly restricts the zero-alloc containment path to compatible comparers, and mismatched-comparer cases fall through to the allocating path preserving prior semantics. The empty-origin handling moved to the public entry point is equivalent. Verdict: LGTM.
Detailed Findings
No functional issues found. The only outstanding item from the prior review—the mis-indented switch (other) line—has been fixed in this increment.
Assessment History
- review 4730525611 reviewed commit
e76ce57: verdict LGTM (once the formatting nit was addressed). Current verdict: LGTM. Assessment changed only in that the previously-flagged cosmetic indentation issue is now resolved by commitd5da427; motivation, approach, and risk are otherwise unchanged.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 44.7 AIC · ⌖ 9.67 AIC · ⊞ 10K
aw0lid
commented
Jul 23, 2026
Friendly ping @dotnet/area-system-collections - Re-requesting review on this PR. All CI checks are green. |
Part of #127279
Summary
ImmutableHashSet<T>.IsProperSubsetOfalways creates a new intermediateHashSet<T>for theothercollection, leading to avoidable allocations and GC pressure, especially for large datasetsOptimization Logic
O(1) Pre-Scan: Immediately returns false if other is an
ICollectionwith a smaller or equal Count. By performing this validation upfront, the need for tracking variables likematchesandextraFoundis eliminated, as any complete match is now mathematically guaranteed to be a proper subset.Fast-Path Pattern Matching: Detects
ImmutableHashSet<T>andHashSet<T>to bypass intermediate allocations.Comparer Guard: Validates
EqualityComparercompatibility before triggering fast paths to ensure logical consistency.Short-Circuit Validation: Re-validates$O(n)$ enumeration.
Countwithin specialized paths for an immediate exit beforeDon't repeat your self: reused$O(1)$ lookup efficiency when other is a
SetEqualsWithHashsetandSetEqualsWithImmutableHashsetmethods to avoid code duplication while ensuring we leverage theHashset<T>.Zero-Allocation Execution: Direct iteration over compatible collections, eliminating the costly
new HashSet<T>(other)fallback.Deferred fallback: Reserves the expensive allocation solely for general
IEnumerabletypes.Click to expand Benchmark Source Code
Click to expand Benchmark Results
Benchmark Results (Before Optimization)
Benchmark Results (After Optimization)
Performance Analysis Summary (100,000 Elements)
✅ Unit Tests Added
Added unit tests for
IsProperSubsetOfto cover various edge cases and ensure the correctness of the new logic:Ordinalvs.OrdinalIgnoreCase).ICollection<T>logic to ensure that collections with duplicates are handled correctly by the early exit (Count <= origin.Count).