8307538: Memory leak in TreeTableView when calling refresh #1129
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
will separate Monkey Tester changes into a separate PR. |
|
/reviewers 2 |
|
@kevinrushforth |
|
I've seen the problem with Views and Cell creation often enough now that I think we may want to talk about the fundamental problem that is causing these issues. The lifecycle of a Cell or Row is not the same as the View that contains them. Cells are created on demand, and should either be destroyed or unlinked. In JavaFX, we have chosen to not have an explicit In other words, a Specifically for this PR, only the listeners registered as part of |
|
This is a very good suggestion, thanks! edit: For the purposes of fixing the regression bug, I would like to proceed with this fix as is. |
Yes, and we wouldn't do this as part of a regression bug fix like this. It seems worth filing a follow-up bug for this. An explicit life-cycle that eliminates the need for weak listeners is generally a good thing.
Agreed. |
There was a problem hiding this comment.
If I run just the TreeTableRowSkinTest test class, I now get a test failure:
$ gradle --info -PTEST_ONLY=true :controls:test --tests TreeTableRowSkinTest
TreeTableRowSkinTest > treeTableRowWithFixedCellSizeShouldIgnoreVerticalPadding() FAILED
org.opentest4j.AssertionFailedError: expected: <18.0> but was: <23.0>
at app//org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at app//org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62)
at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:86)
at app//org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:81)
at app//org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1010)
at app//test.javafx.scene.control.skin.TreeTableRowSkinTest.treeTableRowWithFixedCellSizeShouldIgnoreVerticalPadding(TreeTableRowSkinTest.java:251)
Given that the GHA run passes, as does a full controls test run on my system, that suggests something odd is going on with that test, possibly it is affected by tests in another class. If so, then it could fail randomly anyway, since the tests are run in an unspecified (and unpredictable) order.
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java
Outdated
Show resolved
Hide resolved
|
@aghaisas can you review this? |
I think that's fine, but I do think you should remove the changes that don't contribute to the fix (I'm pretty sure the weak listeners in the constructor are not a problem). |
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
Outdated
Show resolved
Hide resolved
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
Outdated
Show resolved
Hide resolved
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
Outdated
Show resolved
Hide resolved
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java
Outdated
Show resolved
Hide resolved
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
I left a couple inline comments.
I also wonder whether the index and treeItem properties need to be weak (I don't think they do).
The failing test is something that will need to be tracked down. If I revert your changes to treeTableRowWithFixedCellSizeShouldIgnoreVerticalPadding, the test passes when that class is run by itself, but fails when the entire controls test suite is run. The modified test fails with that class is run by itself, but passes when the entire controls test suite is run.
This suggests a possible problem where the (now weak) listeners are getting GCed sometimes but not others, and that whether or not it is GCed causes differences that are visible to the test. So either the test is relying on an implementation detail, or there is a functional problem caused by the listeners being GCed too early or not early enough (the latter could point to the need to remove them in dispose).
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
Outdated
Show resolved
Hide resolved
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java
Outdated
Show resolved
Hide resolved
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java
Show resolved
Hide resolved
|
it looks like the test is unstable - it works fine in Eclipse, but sometimes fails with gradle. investigating. |
It's definitely affected by GC, so I'd start by looking there. I can change whether the the test passes or fails by changing the heap size or adding a call to gc before each test. |
| ListenerHelper lh = ListenerHelper.get(this); | ||
|
|
||
| lh.addChangeListener(control.indexProperty(), (ev) -> { | ||
| registerChangeListener(control.indexProperty(), (x) -> { | ||
| updateCells = true; | ||
| }); | ||
|
|
||
| lh.addChangeListener(control.treeItemProperty(), (ev) -> { | ||
| registerChangeListener(control.treeItemProperty(), (obs) -> { | ||
| updateTreeItem(); | ||
| // There used to be an isDirty = true statement here, but this was | ||
| // determined to be unnecessary and led to performance issues such as |
There was a problem hiding this comment.
These changes don't fail the test when I undo them. As said before, they're unnecessary.
Reasoning: they register on TreeTableRow, which is associated with the skin directly. If their lifecycles don't match, then dispose will take care of unregistering. If their lifecycles do match, then they go out of scope at the same time.
Unless you prefer using the register functions, I think this change should be undone.
There was a problem hiding this comment.
Even though I don't particularly like register*Listener() because of its asymmetric nature (when it comes to removing listeners), here we do need to create weak listeners that get unregistered upon dispose(). We need weak listeners because TreeTableView does not explicitly "disconnect" unused cells (e.g. refresh()), and we need dispose() due to skin life cycle.
So, in this particular case, I think this change should be ok.
There was a problem hiding this comment.
Sorry, I think we may be misunderstanding each other. I'm specifically talking about the two listeners added in TreeTableRowSkin's constructor on indexProperty and treeItemProperty. These are part of the TreeTableRow which is also discarded when refresh is called.
-
The test passes if these changes are reverted. This is a clear indication that either the test is insufficient, or that your assumption, that these must be weak, is incorrect. If you can construct a test that requires these listeners to be weak, then I think the changes are warranted.
-
At the risk of stating the obvious, the listeners in the constructor added using
ListenerHelperare also correctly removed indispose.ListenerHelperdoes this in a similar way to theregistermethods. -
The
indexPropertyandtreeItemPropertylisteners are attached to theTreeTableRow, not theTreeTableView. The refresh function replaces the entire row, including the skin. There is no need to use weak listeners for this purpose. Compare this to the listeners that are attached to theTreeTableViewor theVirtualFlow; these have a much longer lifecycle and can survive multiple refreshes and row factory replacements, and hence should be weak (for now).
There was a problem hiding this comment.
you are right, of course. and I should know - ListenerHelper is my baby. I guess the fear of another regression overtook me.
reverting.
thank you for your patience and persistence!
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
Outdated
Show resolved
Hide resolved
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
Outdated
Show resolved
Hide resolved
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private void updateCachedFixedSize() { | ||
| if (getSkinnable() != null) { | ||
| TreeTableView<T> t = getSkinnable().getTreeTableView(); |
There was a problem hiding this comment.
I know this is a short method, but I would rather see a more descriptive variable name here
There was a problem hiding this comment.
I prefer not to drag long names if a very descriptive type is right there: TreeTableView<T> t
There was a problem hiding this comment.
Looks good.
@hjohn Did you want to re-review (there was only one cleanup change since you approved)?
|
@andy-goryachev-oracle 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 21 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
| TreeTableView<T> treeTableView = getSkinnable().getTreeTableView(); | ||
| if (treeTableView == null) { | ||
| lh.addInvalidationListener(getSkinnable().treeTableViewProperty(), (ev) -> { | ||
| registerInvalidationListener(getSkinnable().treeTableViewProperty(), (x) -> { |
There was a problem hiding this comment.
One question here: Why does this prevent the leak but the ListenerHelper does not?
There was a problem hiding this comment.
The difference is that registerInvalidationListener() adds a weak listener, while ListenerHelper adds a strong listener.
It is possible to use ListenerHelper here, at the expense of more complicated code since we'd need to explicitly disconnect the listener when tableViewProperty value gets set.
Another solution would involve adding a method to add a weak listener to the ListenerHelper to avoid explicit cleanup, or
Go back to the original code which used register/unregister*Listener in this particular case.
|
thank you all for your feedback and effort! |
|
/integrate |
|
Going to push as commit 17ed2e7.
Your commit was automatically rebased without conflicts. |
|
@andy-goryachev-oracle Pushed as commit 17ed2e7. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Fixed a memory leak in TreeTableView by reverting to register**Listener (which is ok in this particular situation) - the leak is specific to TreeTableRowSkin.
Added a unit test.
Progress
Issue
"2")Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1129/head:pull/1129$ git checkout pull/1129Update a local copy of the PR:
$ git checkout pull/1129$ git pull https://git.openjdk.org/jfx.git pull/1129/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1129View PR using the GUI difftool:
$ git pr show -t 1129Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1129.diff
Webrev
Link to Webrev Comment