RFR: 8244297: memory leak test utility [v6]
Ambarish Rapte
arapte at openjdk.java.net
Thu Oct 8 07:59:55 UTC 2020
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
More information about the openjfx-dev
mailing list