On Mon, 18 Nov 2024 16:19:20 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> There are multiple issues in the `TableMenu` logic that result in a memory >> leak. >> >> The following problems are now fixed with this PR: >> - The listener, that is registered to the `col.visibleProperty()` was not >> weak nor was it ever unregistered >> - The fix is to do pretty much the same that was already done with the >> `col.textProperty()` listener >> - The `col.visibleProperty()` and `col.textProperty()` where added again and >> again and again to the column when the columns changed (which happens when >> toggling the visibility). >> - The fix is to add the listeners once - when the `MenuItem` is created. >> This way, when the `TableMenu` is rebuild and the cached `MenuItem` is used, >> the whole listener logic is not run again for the column > > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableViewTableHeaderRowTest.java > line 261: > >> 259: /** >> 260: * Tests that toggling the column visibility does not cause memory >> leaks. >> 261: * See also: <a >> href="https://bugs.openjdk.org/browse/JDK-8341687";>JDK-8341687</a>. > > same comment - does not test for memory leak. the memory leak test might > involve `JMemoryBuddy` - see for example `SystemMenuBarTest`. I tried, but it isn't that easy - since the memory is not leaked because we keep some references we should not keep - rather we are adding listener again and again and again. So we can not assert things to be collectable, as they should not be, still they increase the memory footprint instead. So I did not manage to make a test with `JMemoryBuddy`, but can prove the point with the amount of listener instead. I wrote a test that checked, that the memory is similar high as before, that worked as well but I was afraid that is might be flaky, which I want to avoid. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1640#discussion_r1847209388
