RFR: 8244297: Provide utility for testing for memory leaks [v10]

[Kevin Rushforth]

On Sun, 25 Oct 2020 13:24:44 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

>> It's based on the discussion of my previous PR: https://github.com/openjdk/jfx/pull/71
>> 
>> I Added test utility class copied from JMemoryBuddy and used it to simplify 4 of the existing unit tests.
>> 
>> It's a direct copy of my project [JMemoryBuddy](https://github.com/Sandec/JMemoryBuddy) without any changes.
>> I'm also using it in most of the projects I'm involved with and in my experience, the tests with this Library are very stable. I can't remember wrong test results. Sometimes the memory behaviour of some libraries itself is not stable but the tests with JMemoryBuddy are basically always correct.
>
> Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8244297
>   Added some more whitespaces based on review

Thanks for making the suggested changes. I left one more comment inline and a couple very minor points (which I only mention as something you might want to fix when you take care of the one issue I raised).

The rest looks good. I see that you modified the InitialNodesMemoryLeakTest to use JMemeoryBuddy. In order to verify that the assertions are working, I injected an error by reverting the fix for the [JDK-8216377](URL). The modified test still correctly catches the bug:

$ gradle --info -PFULL_TEST=true cleanTest :systemTests:test --tests InitialNodesMemoryLeakTest

...
test.javafx.scene.InitialNodesMemoryLeakTest > testRootNodeMemoryLeak STANDARD_OUT
    No Heapdump was created. You might want to change the configuration to get a HeapDump.

Gradle Test Executor 1 finished executing tests.

> Task :systemTests:test FAILED

test.javafx.scene.InitialNodesMemoryLeakTest > testRootNodeMemoryLeak FAILED
    java.lang.AssertionError: Content of WeakReference was not collected. content: Group at 149ab2ce
        at test.util.memory.JMemoryBuddy.assertCollectable(JMemoryBuddy.java:91)
        at test.javafx.scene.InitialNodesMemoryLeakTest.testRootNodeMemoryLeak(InitialNodesMemoryLeakTest.java:89)

1 test completed, 1 failed

Good.

modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 134:

> 132: 
> 133:     /**
> 134:      * Checks whether the content of the WeakReference can not be collected.

Minor: cannot is one word.

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.

modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java line 216:

> 214:         }
> 215: 
> 216: 

Minor: there are extra blank lines here.

-------------

PR: https://git.openjdk.java.net/jfx/pull/204