On Thu, 27 Mar 2025 02:10:40 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> `ForceGC` is already waiting for `trappedCount > 0`. And I would point out
>> that `ForceGC` will be making several `System.gc()` calls, compared to just
>> one in the original version of the test.
>>
>> But if there's concern that we won't reach the needed state during
>> `ForceGC`'s default timeout, I can call `ForceGC.waitFor()` with a longer
>> timeout. `60s` ought to be plenty.
>>
>> What do you think?
>
>> But if there's concern that we won't reach the needed state during ForceGC's
>> default timeout, I can call ForceGC.waitFor() with a longer timeout. 60s
>> ought to be plenty.
>
> I think using 60 seconds as a timeout for `ForceGC` would be good. Also just
> for debugging, in case the test fails for some intermittent reason, I think
> it would be a good idea to print the return value of the `ForceGC` call when
> it returns `false` indicating that the condition didn't satisfy after the
> wait time.
Change updated.
It's a good thing you suggested to print out the ForceGC result. It turns out
that the the ForceGC conditions weren't met, but the test was still passing!
`PhantomReference` is the wrong type of reference for this purpose. From
`java.lang.ref` package doc:
>An object is phantom reachable if it is neither strongly, softly, nor weakly
>reachable, **_it has been finalized_**, and....
The `PhantomReference`s were never being cleared, because nothing could get
finalized.
`WeakReference`s are what's needed ("When the weak references to a
weakly-reachable object are cleared, **_the object becomes eligible for
finalization_**.").
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24143#discussion_r2017778051
