New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8296919: Make system tests that detect memory leaks more robust by using JMemoryBuddy utility #1121
Conversation
Based on preliminary patch by @aghaisas
👋 Welcome back lkostyra! A progress list of the required criteria for merging this PR into |
Webrevs
|
In addition to the manual test, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, with minor suggestions. What do you think?
attemptGCSwingNode(); | ||
assertEquals(TOTAL_SWINGNODE, getCleanedUpSwingNodeCount()); | ||
|
||
for (WeakReference<SwingNode> ref : weakRefArrSN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should add a utility method to JMemoryBuddy to operate on an array/collection, so we don't have to repeat this code again and again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
assertEquals(TOTAL_SWINGNODE, getCleanedUpJLabelCount()); | ||
SwingUtilities.invokeAndWait(() -> Util.sleep(500)); | ||
|
||
for (WeakReference<SwingNode> ref : weakRefArrSN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as before about a new utility method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Assert.assertNull("Couldn't collect TextField", textFieldWeakRef.get()); | ||
|
||
JMemoryBuddy.assertCollectable(tabWeakRef); | ||
JMemoryBuddy.assertCollectable(textFieldWeakRef); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps the utility method operating on an array should accept varargs, like so:
assertCollectable(WeakRef a, WeakRef ... b)
(to differentiate it from a single arg method, and to make sure there are 2+ args)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* JMemoryBuddy now has helpers for arrays of WeakReference, ArrayList of WeakReference and vararg WeakReferences * Updated copyright years in some files which were missed in previous changes
I actually missed that. I looked briefly into it and I agree it is a larger change, I will file a follow-up bug for it after this PR is merged. |
} | ||
} | ||
|
||
public static <T> void assertCollectable(ArrayList<WeakReference<T>> weakReferences) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we change the argument to Collection<...>?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
Thread.sleep(SLEEP_TIME); | ||
for (WeakReference ref : willGC) { | ||
JMemoryBuddy.assertCollectable(ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use newly added methods here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done + others.
|
||
Thread.sleep(SLEEP_TIME); | ||
for (WeakReference ref : willGC) { | ||
JMemoryBuddy.assertCollectable(ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, new methods?
|
||
Thread.sleep(SLEEP_TIME); | ||
for (WeakReference ref : willGC) { | ||
JMemoryBuddy.assertCollectable(ref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
* Changed ArrayList<> to Collection<> in one of assertCollectable-s * Updated LeakTest to use new assertCollectable helpers
The changes look good!
To fix that, could you please modify line 45 of modules/web/.classpath file to this (er, better include the whole file):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, one .classpath file change is requested.
Done, thanks for checking! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes generally look good, although I left a comment about sleeping on the EDT.
|
||
attemptGCJLabel(); | ||
assertEquals(TOTAL_SWINGNODE, getCleanedUpJLabelCount()); | ||
SwingUtilities.invokeAndWait(() -> Util.sleep(500)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sleeping on either the JavaFX Application thread or the Swing EDT (as in this case) is not generally desirable. What is the sleep trying to achieve? Is there a better way, possibly sleeping on the test thread and then doing a SwingUtilities.invokeAndWait
(maybe with a no-op runnable? not sure)?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't believe I missed that!
I wonder if we could use CountDownLatch instead here and use Util.await(CountDownLatch) or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that came from the original patch - I'm no Swing expert so I originally figured I should leave it here. My only guess was that it (among other Thread.sleep()
-s in this patch) is trying to let the other threads run for a bit and chew through the resources to make sure they're collectable.
Upon closer inspection it looks like JMemoryBuddy is already doing those sleeps, so I'll remove them* - tests should work fine without those.
* EDIT: them == sleeps done right before JMemoryBuddy calls
JMemoryBuddy performs sleeps to check if objects are collectable by itself, there is no reason to do those.
I removed the sleeps, ran the test multiple times on all platforms and didn't notice any abnormalities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw a one-time failure that suggests waiting for the EDT to run something, and possibly a sleep, might still be needed.
@@ -77,8 +77,6 @@ public void testSwingNodeMemoryLeak() throws InterruptedException, InvocationTar | |||
testSwingNodeObjectsInStage(); | |||
}); | |||
|
|||
SwingUtilities.invokeAndWait(() -> Util.sleep(500)); | |||
|
|||
JMemoryBuddy.assertCollectable(weakRefArrSN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my Mac (MacBook Pro x64 running macOS 13.2.1) I saw a one-time test failure here. So there might be a need to wait for the EDT to at least do something, possibly followed by a sleep (on the test thread, not on the EDT).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I will add a runAndWait()
noop and a test thread sleep then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean SwingUtilities::invokeAndWait
, right?
Btw, I also got a similar one-time failure in SwingNodeDnDMemoryLeakTest
, so perhaps the same could be added there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I meant invokeAndWait
. A force of habit from using our Util
class...
I also added it to SwingNodeDnDMemoryLeakTest
just to be sure. After a few runs it works fine on my end (but also, it did earlier so I can't really fully test it).
On some platforms Swing memory leak tests occasionally failed, this might help to mitigate this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@lukostyra 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:
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 7 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the 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 (@andy-goryachev-oracle, @kevinrushforth) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
@andy-goryachev-oracle I'll wait for your last look at it before I integrate it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor javadoc changes, I'll re-approve if you want to make them.
@@ -91,6 +92,38 @@ public static void assertCollectable(WeakReference weakReference) { | |||
} | |||
} | |||
|
|||
/** | |||
* Checks whether an array of WeakReference objects can be collected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should rather say something like "whether content of all references in the array", rather than array itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (+ others)
} | ||
|
||
/** | ||
* Checks whether a Collection of WeakReference objects can be collected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment: "content of all references in a collection"
} | ||
|
||
/** | ||
* Checks whether provided WeakReference objects can be collected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment: "content of references..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
/integrate |
@lukostyra |
/sponsor |
Going to push as commit d9a82d1.
Your commit was automatically rebased without conflicts. |
@andy-goryachev-oracle @lukostyra Pushed as commit d9a82d1. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Verified these changes on all platforms and saw no abnormalities in test execution.
This change is based on a preliminary patch done by @aghaisas , with some minor changes and the addition of one Leak-related web test. EventListenerLeak test was not touched, as it is a separate manual test app instead of a JUnit test.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1121/head:pull/1121
$ git checkout pull/1121
Update a local copy of the PR:
$ git checkout pull/1121
$ git pull https://git.openjdk.org/jfx.git pull/1121/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1121
View PR using the GUI difftool:
$ git pr show -t 1121
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1121.diff
Webrev
Link to Webrev Comment