WeakHashMap enhancement and expand #2632
Conversation
|
|
| import shark.LeakTraceObject.ObjectType.INSTANCE | ||
|
|
||
| class AndroidObjectInspectorsTest { | ||
|
|
||
| // TODO: There is another trace with the extended WeakHashMap. |
There was a problem hiding this comment.
Trace before
Trace after
There was a problem hiding this comment.
This is happening because this change changed the expected behavior for weak keys, making them look like strong keys. That won't work.
| } | ||
| assertThat(recomposerNode.originObject.leakingStatus == NOT_LEAKING) | ||
| assertThat(recomposerNode.originObject.leakingStatusReason == "Recomposer is in state PendingWork") | ||
| assertThat(recomposerNode.originObject.leakingStatus).isEqualTo(UNKNOWN) |
There was a problem hiding this comment.
It was not executed, I also fixed it
|
|
||
| val hashMapClassId = weakHashMapClass.objectId | ||
|
|
||
| return InternalSharedHashMapReferenceReader( |
There was a problem hiding this comment.
Unfortunately, we need more tweaking than this. WeakHashMap has weak keys. We need to reconnect this with how we define which references to follow or not.
EpicDima
force-pushed
the
epicdima/WeakHashMap_enhancement
branch
from
7370563
to
f9be8b9
Compare
EpicDima
force-pushed
the
epicdima/WeakHashMap_enhancement
branch
from
f9be8b9
to
963d25a
Compare
| assertThat(refPath).hasSize(3) | ||
|
|
||
| with(refPath[0]) { | ||
| assertThat(owningClassName).isEqualTo(WeakHashMap::class.qualifiedName) |
There was a problem hiding this comment.
This is failing in CI
shark.OpenJdkInstanceRefReadersTest > WeakHashMap expanded FAILED
org.junit.ComparisonFailure: expected:<"java.[util.WeakHashMap]"> but was:<"java.[lang.ref.Reference]">
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
at shark.OpenJdkInstanceRefReadersTest.WeakHashMap expanded(OpenJdkInstanceRefReadersTest.kt:432)
|
Thanks, I do want to take a closer look, check out the branch locally, figure out why it's failing etc. |
|
The failure has to do with the dynamic nature of WeakHashMap: the tests are adding a non retained key the map, so depending on how much work the GC is doing, sometimes the entry will still be in the map and sometimes not. |
… value in the map case
- Change tests to leverage a retained key rather than a weakly reachable one, which created flakiness and wasn't testing the intended behavior (a cleared weak key sees the entry added to the reference queue but we don't want to find objects there anyway) - Removed tests that didn't seem ended (we're mostly interested in the expander case, and I didn't get what the singleton case is supposed to be about)
|
Updated the branch with cleanups and fixes. |
#2463 solution
I updated the PR, but the CI check has failed, it works locally, even with JDK 17 Zulu, which is used on CI. Maybe it's the wrong implementation again, if so, then it's probably possible to close the PR, since now I don't have the competence to make the right solution.