RFR: 8244297: Provide utility for testing for memory leaks [v10]
Kevin Rushforth
kcr at openjdk.java.net
Mon Oct 26 23:25:21 UTC 2020
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
More information about the openjfx-dev
mailing list