Stop double counting self-referenced classes #4955
Conversation
| @@ -164,15 +164,18 @@ class ObjectSetDiff { | |||
| if (inBefore && inAfter) continue; | |||
|
|
|||
| final object = before.objectsByCodes[code] ?? after.objectsByCodes[code]!; | |||
| final excludeFromRetained = | |||
There was a problem hiding this comment.
this part is subtle. comment on why the same excludeFromRetained value is used for all cases.
There was a problem hiding this comment.
refactored for readability
There was a problem hiding this comment.
Did you upload the refactor?
What I need is a comment explaining the invariants that make this logic correct.
Is the deal that there should really be a single retained excludedFromRetained set used for both before and after or it it something different?
There was a problem hiding this comment.
if (inBefore && inAfter) continue;
In this code any object is either in before or in after. Never together.
It is obvious for those who read the method from the beginning.
| @@ -194,19 +204,27 @@ class ObjectSet extends ObjectSetStats { | |||
| static ObjectSet empty = ObjectSet()..seal(); | |||
|
|
|||
| final objectsByCodes = <IdentityHashCode, AdaptedHeapObject>{}; | |||
| final excludedFromRetained = <IdentityHashCode>{}; | |||
There was a problem hiding this comment.
nit: is this "excludedFromRetained" or "alreadyIncludedInRetained" or similar?
|
|
||
| final theClass = objects.last.heapClass; | ||
|
|
||
| final firstWithSameClass = |
There was a problem hiding this comment.
This is simpler and I think the intent is a bit clearer if rewritten as:
if (objects.length < 2) return false;
return objects.take(objects.length - 1).any((object) => object.heapClass == theClass);
There was a problem hiding this comment.
take will create one more array
Knowing that the method will run for every object, I would avoid it.
Updated early return.
There was a problem hiding this comment.
actually, I see 'take' returns itarable
|
|
||
| final path = data.retainingPath(objectIndex); | ||
| objects.countInstance( | ||
| object, | ||
| excludeFromRetained: path?.isRetainedBySameClass ?? false, |
No description provided.