On Mon, 8 May 2023 18:55:38 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Fixed a memory leak in TreeTableView by using WeakInvalidationListener >> (which is the right approach in this particular situation) - the leak is >> specific to TreeTableRowSkin. >> >> Added a unit test. >> >> Added Refresh buttons and Skin menu to the Monkey Tester (will expand these >> to other controls as a part next MT update). > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - removed monkey tester changes > - Merge remote-tracking branch 'origin/master' into 8307538.refresh > - whitespace > - updated test > - variable tree cell fix > - fixed size selector > - list view page > - fix and test > - skin menu > - refresh button modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 101: > 99: control.indexProperty().addListener(new > WeakInvalidationListener((x) -> { > 100: updateCells = true; > 101: })); I don't think this needed changing, the index property is directly available on `TreeTableRow` and so shouldn't cause GC issues. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 108: > 106: // determined to be unnecessary and led to performance > issues such as > 107: // those detailed in JDK-8143266 > 108: })); As above, I don't think this needs to be weak. modules/javafx.controls/src/main/java/javafx/scene/control/skin/TreeTableRowSkin.java line 334: > 332: private void updateTreeItem() { > 333: unregisterInvalidationListeners(graphicProperty()); > 334: treeItem = (getSkinnable() == null) ? null : > getSkinnable().getTreeItem(); This is not needed if the weak listener change is reverted in the constructor. The cause for it being `null` is that the listener is now no longer removed by `dispose` where before (with `ListenerHelper` or `registerXXXListener`) it would have been. Also, I think when `dispose` is called, this skin should still do its utmost best to remove all listeners immediately. modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TreeTableRowSkinTest.java line 335: > 333: > 334: treeTableView.refresh(); > 335: Toolkit.getToolkit().firePulse(); I think cells should also be collectable in other situations, like replacing the row factory. Is it worth adding a test for that? ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189429842 PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189430267 PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189434672 PR Review Comment: https://git.openjdk.org/jfx/pull/1129#discussion_r1189438781