8244297: Provide utility for testing for memory leaks #204

FlorianKirmaier · 2020-05-03T21:39:41Z

It's based on the discussion of my previous PR: #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 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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8244297: Provide utility for testing for memory leaks

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Ambarish Rapte (@arapte - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/204/head:pull/204
$ git checkout pull/204

Added test utility class copied from JMemoryBuddy and used it to simplify 4 of the exiting unit tests.

bridgekeeper · 2020-05-03T21:41:34Z

👋 Welcome back fkirmaier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

mlbridge · 2020-05-03T22:23:32Z

Webrevs

kevinrushforth · 2020-05-04T13:46:23Z

/reviewers 2

openjdk · 2020-05-04T13:46:37Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

kevinrushforth

I'll put this on my queue to review and test it. I left some high-level comments below.

modules/javafx.base/src/test/java/de/sandec/jmemorybuddy/JMemoryBuddy.java

kevinrushforth · 2020-05-04T13:57:53Z

modules/javafx.base/src/test/java/de/sandec/jmemorybuddy/JMemoryBuddy.java

+    }
+
+
+    public static void createHeapDump() {


I don't think we want automatic creation of heap dumps by our unit test suite. That would need a larger discussion. I recommend to either comment this out, or else to qualify it with a system property that could be passed in via a gradle property (e.g., as is done for UNSTABLE_TEST), with a default of false.

I've made it configurable in the Library with system-parameters and I've changed the default not create a heap dump.
Is that ok?

Yes, that seems fine.

modules/javafx.base/src/test/java/de/sandec/jmemorybuddy/JMemoryBuddy.java

FlorianKirmaier · 2020-05-05T10:38:56Z

I would suggest making everything configurable in the original library which will be suggested here to be changed.
I will now add the possibility to make the creation of the heapDump configurable, and also the createGarbage method.
If something should behave differently by default, then I will just adjust the default values for this copy of the library.

I will improve the documentation in the original Library. How should I add the documentation to the JavaFX project? Ideally, I would just reference somewhere to the documentation of the original library. How/Where should I do that?

FlorianKirmaier · 2020-05-05T10:57:23Z

It's also worth mentioning that this utility allows checking whether an object can not be collected. This is important because the applications, which are using WeakReferences/WeakListeners often introduce bugs that are hard to debug because they only happen after the GC has been run.

Updated JMemoeryBuddy Added Copyright header disabled default heapdump

Added some javadoc to the testutility Some methods/fields are now private

FlorianKirmaier · 2020-05-05T20:39:18Z

I've now added also some JavaDoc to the public methods.

jskov-jyskebank-dk · 2020-05-06T06:06:17Z

Hi @jskovjyskebankdk, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user jskovjyskebankdk for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

Improved the JavaDoc for JMemoryBuddy

jskov · 2020-05-07T16:58:52Z

What I tried asking from my work account is: what is the purpose of createGarbage?

FlorianKirmaier · 2020-05-07T18:16:22Z

The createGarbage method stimulates the GC. All unit tests are green without it, but according to my memory some tests in an earlier version of this library were unstable without this.
I can also change the configuration to create 0 garbage.

FlorianKirmaier · 2020-07-21T12:27:05Z

Any news about this PR?

kevinrushforth · 2020-07-23T17:44:14Z

Now that jfx15 is pretty much done, we'll put this back on the queue to review.

FlorianKirmaier · 2020-09-10T13:06:04Z

Any news? I will probably soon have some PR which would like to use it for the tests.

arapte

Tests fail to compile, please correct the import.
Pointed error in ToggleButtonTest, please check other files also when correcting this.

modules/javafx.controls/src/test/java/test/javafx/scene/control/ToggleButtonTest.java

Fixing some wrong imports

FlorianKirmaier · 2020-09-30T11:43:34Z

I've fixed the wrong imports. It would be great if Travis could check whether all systemTests are compiling.

kevinrushforth · 2020-09-30T12:36:50Z

Thanks, we'll review it now.

Btw, we don't plan to hook up Travis, but instead will be looking at GitHub actions now that the jdk project is using them successfully.

kevinrushforth · 2020-10-01T17:29:34Z

It looks like you are missing an import statement in one of the tests:

> Task :systemTests:compileTestJava
...\jfx\tests\system\src\test\java\test\javafx\scene\InitialNodesMemoryLeakTest.java:88: error: cannot find symbol
        JMemoryBuddy.assertCollectable("groupWRef");
        ^
  symbol:   variable JMemoryBuddy
  location: class InitialNodesMemoryLeakTest
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
1 error

You can run the following locally on your system once you have fixed this to verify that there are no others.

gradle --continue -PFULL_TEST=true test

Fixed unit-test

FlorianKirmaier · 2020-10-03T16:18:44Z

./gradlew --continue -PFULL_TEST=true test Seems to be very unstable on my machine (Mac). It produces various errors, which are not related to my changes, which makes it basically useless in my environment.

I've checked the 4 changed tests with the following commands:
./gradlew --continue -PFULL_TEST=true controls:test
./gradlew --continue -PFULL_TEST=true systemTest:test --tests "*VirtualFlowMemoryLeakTest*"
./gradlew --continue -PFULL_TEST=true systemTest:test --tests "*ProgressIndicatorLeakTest*"
./gradlew --continue -PFULL_TEST=true systemTest:test --tests "*InitialNode*"
They all compile now and are green.

arapte

I have reviewed only in perspective of Java coding guidelines. I still have to review the functionality.
Most of the comments are minor like variable name change and typos. I would recommend you to please go through all of the code to find and fix any other corrections related to Coding guidelines.

modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java

arapte · 2020-10-08T07:31:03Z

modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java

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


Seems like candidate for System.err.

That might be true, but honestly, I'm using System.err so rarely (never) that I have a better feeling avoiding it altogether and eliminating the chance for rare cornercases.

modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java

openjdk · 2020-10-20T07:06:51Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

openjdk · 2020-10-20T07:08:45Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

openjdk · 2020-10-20T07:17:28Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

openjdk · 2020-10-20T07:19:08Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

Updated JMemoryBuddy based on codereview.

FlorianKirmaier · 2020-10-22T20:00:54Z

First, thank you both for all your comments!
I integrated most of the suggested changes into the class and I will commit them back to the original project when the PR is closed.
I've adopted the whitespace after if, and integrated most of the suggested changes (but not all)!

The title should be correct now.

kevinrushforth

Just a quick note, since I don't have time to review it today. Thanks for making the changes. I see that you added the space after if (except in the new method you added), but not after for and while.

I'll do more testing next week as part of my review.

modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java

Added some more whitespaces based on review

FlorianKirmaier · 2020-10-25T13:21:59Z

Just added some more spaces!

kevinrushforth

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. 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@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

more improvements based on code-review

kevinrushforth

Looks good!

arapte

looks good to me too...

openjdk · 2020-10-30T09:47:46Z

@FlorianKirmaier This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8244297: Provide utility for testing for memory leaks

Reviewed-by: kcr, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 151 new commits pushed to the master branch:

0376e60: 8255487: Mark SandboxAppTest unstable on Windows
7da928b: 8255497: [TestBug] Controls unit tests - clean up unnecessary prints on STANDARD_OUT
4f1eb7d: 8254013: gradle test should run all test classes even if they don't end with "Test"
690d266: 8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException
2c67555: 8213573: MouseLocationOnScreenTest fails intermittently
243b1a5: 8255241: [TestBug] Re-enable few ignored tests in javafx.controls module that pass with latest code
4e5f0e6: 8252596: [TESTBUG] WebPageShim::paint is not thread-safe
a5a71d1: 8247494: Test failure in ImageRaceTest on some systems
ae1fb61: 8255002: Many javafx.controls unit tests have incorrect name containing impl_*
... and 141 more: https://git.openjdk.java.net/jfx/compare/1cae6a87a8322abf96e56853fc8d205723439e42...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @arapte) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

FlorianKirmaier · 2020-10-30T13:19:31Z

/integrate

openjdk · 2020-10-30T13:20:53Z

@FlorianKirmaier
Your change (at version 6ece3a7) is now ready to be sponsored by a Committer.

arapte · 2020-10-30T20:05:59Z

/sponsor

openjdk · 2020-10-30T20:08:18Z

@arapte @FlorianKirmaier Since your change was applied there have been 151 commits pushed to the master branch:

0376e60: 8255487: Mark SandboxAppTest unstable on Windows
7da928b: 8255497: [TestBug] Controls unit tests - clean up unnecessary prints on STANDARD_OUT
4f1eb7d: 8254013: gradle test should run all test classes even if they don't end with "Test"
690d266: 8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException
2c67555: 8213573: MouseLocationOnScreenTest fails intermittently
243b1a5: 8255241: [TestBug] Re-enable few ignored tests in javafx.controls module that pass with latest code
4e5f0e6: 8252596: [TESTBUG] WebPageShim::paint is not thread-safe
a5a71d1: 8247494: Test failure in ImageRaceTest on some systems
ae1fb61: 8255002: Many javafx.controls unit tests have incorrect name containing impl_*
... and 141 more: https://git.openjdk.java.net/jfx/compare/1cae6a87a8322abf96e56853fc8d205723439e42...master

Your commit was automatically rebased without conflicts.

Pushed as commit 1a11334.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8244297

5721de1

Added test utility class copied from JMemoryBuddy and used it to simplify 4 of the exiting unit tests.

FlorianKirmaier changed the title ~~8244297 memory leak test utility~~ 8244297: memory leak test utility May 3, 2020

openjdk bot added the rfr Ready for review label May 3, 2020

kevinrushforth reviewed May 4, 2020

View reviewed changes

FlorianKirmaier added 2 commits May 5, 2020 13:53

JDK-8244297

e7d9ad6

Updated JMemoeryBuddy Added Copyright header disabled default heapdump

JDK-8244297

72d1228

Added some javadoc to the testutility Some methods/fields are now private

JDK-8244297

0852877

Improved the JavaDoc for JMemoryBuddy

kevinrushforth self-requested a review July 23, 2020 17:43

kevinrushforth requested a review from arapte September 10, 2020 13:21

arapte suggested changes Sep 25, 2020

View reviewed changes

modules/javafx.controls/src/test/java/test/javafx/scene/control/ToggleButtonTest.java Outdated Show resolved Hide resolved

modules/javafx.controls/src/test/java/test/javafx/scene/control/ToggleButtonTest.java Show resolved Hide resolved

JDK-8244297

adc870c

Fixing some wrong imports

JDK-8244297

6a861e4

Fixed unit-test

arapte suggested changes Oct 8, 2020

View reviewed changes

FlorianKirmaier changed the title ~~8244297: memory leak test utility~~ 8244297: Provide utility for testing for memory leaks Oct 22, 2020

FlorianKirmaier added 3 commits October 22, 2020 21:21

JDK-8244297

427170b

Updated JMemoryBuddy based on codereview.

JDK-8244297

3f73195

Updated JMemoryBuddy based on codereview.

JDK-8244297

913622b

Updated JMemoryBuddy based on codereview.

kevinrushforth reviewed Oct 24, 2020

View reviewed changes

modules/javafx.base/src/test/java/test/util/memory/JMemoryBuddy.java Outdated Show resolved Hide resolved

kevinrushforth self-requested a review October 24, 2020 18:58

JDK-8244297

91bda93

Added some more whitespaces based on review

kevinrushforth reviewed Oct 26, 2020

View reviewed changes

kevinrushforth self-requested a review October 27, 2020 20:34

JDK-8244297

6ece3a7

more improvements based on code-review

kevinrushforth approved these changes Oct 28, 2020

View reviewed changes

arapte approved these changes Oct 30, 2020

View reviewed changes

openjdk bot added the ready Ready to be integrated label Oct 30, 2020

openjdk bot added the sponsor Ready to sponsor label Oct 30, 2020

openjdk bot closed this Oct 30, 2020

openjdk bot added integrated Pull request has been integrated and removed sponsor Ready to sponsor ready Ready to be integrated rfr Ready for review labels Oct 30, 2020

8244297: Provide utility for testing for memory leaks #204

8244297: Provide utility for testing for memory leaks #204

Conversation

FlorianKirmaier commented May 3, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented May 3, 2020

mlbridge bot commented May 3, 2020 • edited Loading

Webrevs

kevinrushforth commented May 4, 2020

openjdk bot commented May 4, 2020 • edited by bridgekeeper bot Loading

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth May 4, 2020

Choose a reason for hiding this comment

FlorianKirmaier May 5, 2020

Choose a reason for hiding this comment

kevinrushforth May 6, 2020

Choose a reason for hiding this comment

FlorianKirmaier commented May 5, 2020

FlorianKirmaier commented May 5, 2020

FlorianKirmaier commented May 5, 2020

jskov-jyskebank-dk commented May 6, 2020 • edited by bridgekeeper bot Loading

jskov commented May 7, 2020

FlorianKirmaier commented May 7, 2020

FlorianKirmaier commented Jul 21, 2020 • edited Loading

kevinrushforth commented Jul 23, 2020

FlorianKirmaier commented Sep 10, 2020

arapte left a comment

Choose a reason for hiding this comment

FlorianKirmaier commented Sep 30, 2020

kevinrushforth commented Sep 30, 2020

kevinrushforth commented Oct 1, 2020

FlorianKirmaier commented Oct 3, 2020

arapte left a comment

Choose a reason for hiding this comment

arapte Oct 8, 2020

Choose a reason for hiding this comment

FlorianKirmaier Oct 22, 2020

Choose a reason for hiding this comment

openjdk bot commented Oct 20, 2020 • edited by bridgekeeper bot Loading

openjdk bot commented Oct 20, 2020 • edited by bridgekeeper bot Loading

openjdk bot commented Oct 20, 2020 • edited by bridgekeeper bot Loading

openjdk bot commented Oct 20, 2020

FlorianKirmaier commented Oct 22, 2020

kevinrushforth left a comment

Choose a reason for hiding this comment

FlorianKirmaier commented Oct 25, 2020

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 30, 2020 • edited Loading

FlorianKirmaier commented Oct 30, 2020

openjdk bot commented Oct 30, 2020

arapte commented Oct 30, 2020

openjdk bot commented Oct 30, 2020

FlorianKirmaier commented May 3, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented May 3, 2020 •

edited

Loading

openjdk bot commented May 4, 2020 •

edited by bridgekeeper bot

Loading

jskov-jyskebank-dk commented May 6, 2020 •

edited by bridgekeeper bot

Loading

FlorianKirmaier commented Jul 21, 2020 •

edited

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 20, 2020 •

edited by bridgekeeper bot

Loading

openjdk bot commented Oct 30, 2020 •

edited

Loading