RFR: 8244297: memory leak test utility [v6]

[Ambarish Rapte]

On Sat, 3 Oct 2020 16:00:48 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
>   Fixed unit-test

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

> 102:             try {
> 103:                 Thread.sleep(sleepTime);
> 104:             } catch (InterruptedException e) {}

Ideally the exception should never occur, but in case it does then I think the test should be marked as failed.

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

> 130:      * Checks whether the content of the WeakReference can not be collected.
> 131:      * @param weakReference The WeakReference to check.
> 132:      * @return Returns true, when the provided WeakReference can be collected.

needs typo correction: can not be collected

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

> 150:         f.accept(new MemoryTestAPI() {
> 151:             public void assertCollectable(Object ref) {
> 152:                 if(ref == null) throw new NullPointerException();

`Object.requireNonNull()` method would be more suitable choice for null check.

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

> 45:
> 46:     private static int steps = 10;
> 47:     private static int overallTime = 1000;

`overallTime` can be named as `totalDuration` or `testDuration`

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

> 46:     private static int steps = 10;
> 47:     private static int overallTime = 1000;
> 48:     private static int sleepTime = overallTime / steps;

`sleepTime` -> `sleepDuration`

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

> 50:     private static int garbageAmount = 999999;
> 51:     private static String MX_BEAN_PROXY_TYPE = "com.sun.management:type=HotSpotDiagnostic";
> 52:     private static String outputFolderString = ".";

I am not sure of the directory that we use to save any runtime artifacts, but there must be one common directory that
we should use here also.

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

> 54:     static {
> 55:         outputFolderString = System.getProperty("jmemorybuddy.output",".");
> 56:         overallTime = Integer.parseInt(System.getProperty("jmemorybuddy.checktime","1000"));

`sleepTime` should also be recomputed here based on `overallTime`

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

> 49:     private static boolean createHeapdump = false;
> 50:     private static int garbageAmount = 999999;
> 51:     private static String MX_BEAN_PROXY_TYPE = "com.sun.management:type=HotSpotDiagnostic";

`MX_BEAN_PROXY_TYPE` is not constant, so it should not be all capital letters but should be named just like other
variables.

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

> 86:      * @return Returns true, when the provided WeakReference can be collected.
> 87:      */
> 88:     public static boolean checkCollectable(WeakReference weakReference) {

I would prefer a name like `checkIfCollected` or `checkIfGCed`

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

> 82:
> 83:     /**
> 84:      * Checks whether the content of the WeakReference can be collected.

-> Checks whether the content of the WeakReference **is** collected.

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

> 111:         if(weakReference.get() == null && counter < steps / 3) {
> 112:             int percentageUsed = (int) ((steps - counter) / steps * 100);
> 113:             System.out.println("Warning test seems to be unstable. time used: " + percentageUsed + "%");

Seems like candidate for `System.err.`

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

> 77:             AssertCollectable assertCollectable = new AssertCollectable(weakReference);
> 78:             createHeapDump();
> 79:             throw new AssertionError("Content of WeakReference was not collected. content: " + weakReference.get());

Can be replaced with `assertNull`

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

> 253:
> 254:     static class AssertCollectable {
> 255:         WeakReference<Object> assertCollectable;

There is method, class member variable and class itself with the same name `assertCollectable`.
I think they should be named differently, like for instance,
1. Class `AssertCollectable` can be named as `CollectableReferernce` or `ReferenceToBeGC`
2. The member variable `assertCollectable` can be named as just `weakRef` or `reference`.
3. The method `assertCollectable` can be named as `checkIfGCed` or `checkIfCollected`

Similar change for `AssertNotCollectable`

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

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