On Tue, 27 Oct 2020 19:24:35 GMT, Florian Kirmaier <[email protected]>
wrote:
>> modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line
>> 150:
>>
>>> 148: public static boolean checkNotCollectable(WeakReference
>>> weakReference) {
>>> 149: createGarbage();
>>> 150: System.gc();
>>
>> I recommend calling `System.runFinalization();` here.
>>
>> Also, do you think there is value in checking in a loop? Or is that
>> overkill? One drawback of checking in a loop for the negative case is that
>> you end up going through the loop all the way to the end in the expected
>> case where a reference is not collectable, so it might be best to leave it
>> as a single check as you have done.
>
> I've added runFinalization followed with another System.gc (otherwise it
> wouldn't make any difference)
> I'm quite sure it's avoiding an error in the library, but I don't have a
> unit-test for it yet.
>
> It's a tradeoff between speed and reliability. The library tries to avoid
> false-negative test results at all costs.
> false-positive results are tolerated for speed if they are "sufficiently
> reliable".
> The Library has tests for assertNotCollectable, which are executed with every
> commit for different JVM's, and I don't remember any false-positive results
> (which would be visible as a negative test result, because the unit-test
> checks for failing tests). For that reason, we can assume they are
> "sufficiently reliable" despite the theoretical problem.
That sounds fine then.
As soon as you've pushed the follow-on fix (to add the `runFinalization` and a
second `gc`) I'll finish my review.
-------------
PR: https://git.openjdk.java.net/jfx/pull/204
