Re: RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel
On Thu, 26 Mar 2020 11:37:06 GMT, Jeanette Winzenburg wrote: > > Not included would be memory leaks introduced by whacky listening of > ChoiceBoxSkin and TabPaneSkin to properties of the > selectionModel. The former will (most probably) be fixed as a side-effect of > https://bugs.openjdk.java.net/browse/JDK-8087555 (which I intend to take, if > Ajit agrees :), will open a separate issue > for the latter. Please go ahead and take it up. Thanks! Regarding combining other issue. I am OK with the idea. Bug Title, Bug Description and PR Title should be updated accordingly. - PR: https://git.openjdk.java.net/jfx/pull/148
Re: RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel
On Thu, 26 Mar 2020 11:37:06 GMT, Jeanette Winzenburg wrote: >> @arapte @aghaisas - Can you both review this? > > I would like to widen the issue (and change this pull request accordingly) to > include memory leaks of introduced > selection- and focusModels in other controls that are caused by the basically > same reason, that is strong references > via listeners to controls' properties. The fix would be the same as well, > that is wrap the listeners into > weakXXListeners and register these. The models suggested to include here: > - TreeView: selection- and focusModel > - TreeTableView: focusModel > - TabPane: selectionModel > > Not included would be memory leaks introduced by whacky listening of > ChoiceBoxSkin and TabPaneSkin to properties of the > selectionModel. The former will (most probably) be fixed as a side-effect of > https://bugs.openjdk.java.net/browse/JDK-8087555 (which I intend to take, if > Ajit agrees :), will open a separate issue > for the latter. The extended fix here will have tests for replacing > selection/focusModels with/out a skin registered > for all controls having the respective models. Not entirely certain about > the formal procedure (locally fix and tests > are - nearly, pending some cleanup - ready). Simply change titles, rebase and > push again? If Ambarish and Ajit are OK with this, then the procedure would be as you guessed: change the title of both the JBS bug and then this PR and push your new fix (with or without rebasing would be fine). - PR: https://git.openjdk.java.net/jfx/pull/148
Re: RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel
On Thu, 26 Mar 2020 12:25:14 GMT, Kevin Rushforth wrote: >> I would like to widen the issue (and change this pull request accordingly) >> to include memory leaks of introduced >> selection- and focusModels in other controls that are caused by the >> basically same reason, that is strong references >> via listeners to controls' properties. The fix would be the same as well, >> that is wrap the listeners into >> weakXXListeners and register these. The models suggested to include here: >> - TreeView: selection- and focusModel >> - TreeTableView: focusModel >> - TabPane: selectionModel >> >> Not included would be memory leaks introduced by whacky listening of >> ChoiceBoxSkin and TabPaneSkin to properties of the >> selectionModel. The former will (most probably) be fixed as a side-effect of >> https://bugs.openjdk.java.net/browse/JDK-8087555 (which I intend to take, if >> Ajit agrees :), will open a separate issue >> for the latter. The extended fix here will have tests for replacing >> selection/focusModels with/out a skin registered >> for all controls having the respective models. Not entirely certain about >> the formal procedure (locally fix and tests >> are - nearly, pending some cleanup - ready). Simply change titles, rebase >> and push again? > > If Ambarish and Ajit are OK with this, then the procedure would be as you > guessed: change the title of both the JBS bug > and then this PR and push your new fix (with or without rebasing would be > fine). That is good idea to combine all similarly affected selection models. I think rebase won't be required just updating the PR with more commits should be good enough. - PR: https://git.openjdk.java.net/jfx/pull/148
Re: RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel
On Mon, 23 Mar 2020 23:45:55 GMT, Kevin Rushforth wrote: >> I have basically the same comment / question as I asked in #147 >> >> In general, there are two approaches to avoiding listener-related memory >> leaks. One is to use a WeakListener; the other >> is to explicitly remove the listener when the object is removed or otherwise >> no longer needed. >> Using a WeakListener is certainly easier, but runs the risk of the listener >> being removed too early and not cleaning up >> after itself. I'm not suggesting that's the case here, but it is worth >> looking at. > > @arapte @aghaisas - Can you both review this? I would like to widen the issue (and change this pull request accordingly) to include memory leaks of introduced selection- and focusModels in other controls that are caused by the basically same reason, that is strong references via listeners to controls' properties. The fix would be the same as well, that is wrap the listeners into weakXXListeners and register these. The models suggested to include here: - TreeView: selection- and focusModel - TreeTableView: focusModel - TabPane: selectionModel Not included would be memory leaks introduced by whacky listening of ChoiceBoxSkin and TabPaneSkin to properties of the selectionModel. The former will (most probably) be fixed as a side-effect of https://bugs.openjdk.java.net/browse/JDK-8087555 (which I intend to take, if Ajit agrees :), will open a separate issue for the latter. The extended fix here will have tests for replacing selection/focusModels with/out a skin registered for all controls having the respective models. Not entirely certain about the formal procedure (locally fix and tests are - nearly, pending some cleanup - ready). Simply change titles, rebase and push again? - PR: https://git.openjdk.java.net/jfx/pull/148
Re: RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel
On Mon, 23 Mar 2020 23:45:21 GMT, Kevin Rushforth wrote: >> ChoiceBox leaves a memory leak when replacing the selectionModel. Culprit is >> ChoiceBoxSelectionModel which registers >> listener with strong references. Fix is to change these to weak references. >> Added test that fails before and passes after the test. >> >> for convenience, the bug reference >> https://bugs.openjdk.java.net/browse/JDK-8241455 > > I have basically the same comment / question as I asked in #147 > > In general, there are two approaches to avoiding listener-related memory > leaks. One is to use a WeakListener; the other > is to explicitly remove the listener when the object is removed or otherwise > no longer needed. > Using a WeakListener is certainly easier, but runs the risk of the listener > being removed too early and not cleaning up > after itself. I'm not suggesting that's the case here, but it is worth > looking at. @arapte @aghaisas - Can you both review this? - PR: https://git.openjdk.java.net/jfx/pull/148
Re: RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel
On Mon, 23 Mar 2020 16:32:16 GMT, Jeanette Winzenburg wrote: > ChoiceBox leaves a memory leak when replacing the selectionModel. Culprit is > ChoiceBoxSelectionModel which registers > listener with strong references. Fix is to change these to weak references. > Added test that fails before and passes after the test. > > for convenience, the bug reference > https://bugs.openjdk.java.net/browse/JDK-8241455 I have basically the same comment / question as I asked in #147 In general, there are two approaches to avoiding listener-related memory leaks. One is to use a WeakListener; the other is to explicitly remove the listener when the object is removed or otherwise no longer needed. Using a WeakListener is certainly easier, but runs the risk of the listener being removed too early and not cleaning up after itself. I'm not suggesting that's the case here, but it is worth looking at. - PR: https://git.openjdk.java.net/jfx/pull/148
RFR: 8241455: ChoiceBox - memory leak on replacing selectionModel
ChoiceBox leaves a memory leak when replacing the selectionModel. Culprit is ChoiceBoxSelectionModel which registers listener with strong references. Fix is to change these to weak references. Added test that fails before and passes after the test. for convenience, the bug reference https://bugs.openjdk.java.net/browse/JDK-8241455 - Commit messages: - 8241455: ChoiceBox - memory leak on replacing selectionModel Changes: https://git.openjdk.java.net/jfx/pull/148/files Webrev: https://webrevs.openjdk.java.net/jfx/148/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8241455 Stats: 50 lines in 2 files changed: 42 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/148.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/148/head:pull/148 PR: https://git.openjdk.java.net/jfx/pull/148
