Dispose some pooled object instances #7330
Conversation
We Dispose()/Free() other instances of these classes, but not these specific ones. Fix that.
| @@ -68,7 +68,7 @@ public override void Initialize(AnalysisContext context) | |||
|
|
|||
There was a problem hiding this comment.
Does interfaceToGenericInterfaceMapBuilder need to be Dispose()d too?
| @@ -261,7 +261,7 @@ internal ValueContentAbstractValue IntersectLiteralValues(ValueContentAbstractVa | |||
| } | |||
|
|
|||
| // Merge Literals | |||
| var builder = PooledHashSet<object?>.GetInstance(); | |||
| using var builder = PooledHashSet<object?>.GetInstance(); | |||
There was a problem hiding this comment.
This will Dispose() on the error path (return MaybeContainsNonLiteralState) too.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7330 +/- ##
=======================================
Coverage 96.49% 96.49%
=======================================
Files 1443 1443
Lines 345670 345670
Branches 11370 11370
=======================================
+ Hits 333543 333559 +16
+ Misses 9248 9233 -15
+ Partials 2879 2878 -1 |
There was a problem hiding this comment.
I'd like to see this PR make use of ToImmutableAndClear() to avoid unnecessary array copies when data is extracted from the builder.
| @@ -68,7 +68,7 @@ public override void Initialize(AnalysisContext context) | |||
|
|
|||
| if (wellKnownTypeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsIList, out var iListType)) | |||
| { | |||
| var builder = ArrayBuilder<INamedTypeSymbol>.GetInstance(); | |||
There was a problem hiding this comment.
💡 The ToImmutable() line should also change to ToImmutableAndClear()
| @@ -79,7 +79,7 @@ public override void Initialize(AnalysisContext context) | |||
|
|
|||
| if (wellKnownTypeProvider.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemCollectionsICollection, out var iCollectionType)) | |||
| { | |||
| var builder = ArrayBuilder<INamedTypeSymbol>.GetInstance(); | |||
There was a problem hiding this comment.
💡 The ToImmutable() line should also change to ToImmutableAndClear()
| @@ -275,7 +275,7 @@ internal ValueContentAbstractValue IntersectLiteralValues(ValueContentAbstractVa | |||
| } | |||
| } | |||
|
|
|||
| ImmutableHashSet<object?> mergedLiteralValues = builder.ToImmutableAndFree(); | |||
| ImmutableHashSet<object?> mergedLiteralValues = builder.ToImmutable(); | |||
There was a problem hiding this comment.
| ImmutableHashSet<object?> mergedLiteralValues = builder.ToImmutable(); | |
| ImmutableHashSet<object?> mergedLiteralValues = builder.ToImmutableAndClear(); |
We Dispose()/Free() other instances of these classes, but not these specific ones. Fix that.