Repository navigation
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-collections |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "3f72aeff48b6c06df770429e33c17d1173e43f0b",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "71d81c1705aa1f00e0b57073c97efdd5496f6b9f",
"last_reviewed_commit": "3f72aeff48b6c06df770429e33c17d1173e43f0b",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "71d81c1705aa1f00e0b57073c97efdd5496f6b9f",
"last_recorded_worker_run_id": "29725962056",
"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": "3f72aeff48b6c06df770429e33c17d1173e43f0b",
"review_id": 4733125553
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: ImmutableHashSet<T>.IsSubsetOf previously always allocated an intermediate HashSet<T> from other (via new HashSet<T>(other, origin.EqualityComparer)) even when other was already a set, or when a cheap Count-based early exit could prove the result. This produces avoidable allocations and GC pressure for large collections. The PR is part of the broader collection-optimization effort in #127279.
Approach: The method now pattern-matches other against ImmutableHashSet<T>, HashSet<T>, and ICollection<T>. For the two set types with a compatible comparer, it reuses the existing SetEqualsWithImmutableHashset / SetEqualsWithHashset helpers to iterate origin and test membership directly (zero allocation). For any ICollection<T>, an O(1) Count pre-check returns false when other.Count < origin.Count (using < rather than != to tolerate duplicates in non-set collections). The general IEnumerable<T> fallback still builds a HashSet<T>, but now short-circuits on the deduplicated count and delegates to SetEqualsWithHashset.
Summary: The transformation is correct. The same-comparer set paths reduce to "every origin element is contained in other," which is precisely the subset relation, and the other.Count < origin.Count early exit is valid because those collections hold distinct elements under a matching comparer. The mismatched-comparer paths correctly fall through to the comparer-aware fallback while still safely applying the Count lower-bound check — re-hashing other under origin's comparer can only shrink the distinct count, so other.Count < origin.Count still guarantees a non-subset. The ICollection<T> case correctly uses < to account for possible duplicates. Behavior for empty origin, empty other, duplicates, and mismatched comparers is preserved, and the added tests exercise these edge cases (mismatched Ordinal/OrdinalIgnoreCase comparers, duplicate-bearing lists, empty sets, and smaller-count sets). No correctness, thread-safety, or API-surface concerns; the changes are consistent with the analogous SetEquals implementation already in the file. Verdict: LGTM.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 76.7 AIC · ⌖ 10.3 AIC · ⊞ 10K
|
Friendly ping @dotnet/area-system-collections |
|
Hi @aw0lid, apologies for the delay in getting back to you. The team is dealing with other priorities right now so it might take a while before we can take another look at your change. Thanks for your understanding. |
Part of #127279
Summary
ImmutableHashSet<T>.IsSubsetOfalways 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 count than the source.. By performing this validation upfront, the need for tracking variables likematchesis eliminated, as any complete match is now mathematically guaranteed to be a 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 yourself: 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)
Environment
✅ Unit Tests Added
Added unit tests for
IsSubsetOfto 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).