On Fri, 17 Apr 2020 10:22:15 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> @hjohn Thanks for looking into this. It looks like your changes do improve >> the issue with the JavaFX thread being swamped with listener >> de-registrations. Looking at the JavaFX thread in VisualVM, the >> removeListener call has dropped off the hotspots in the same way it did with >> my pull request. >> >> I wasn't fully confident of making changes to the Node hierarchy to remove >> listeners hence why I approached the issue from the other direction i.e. the >> obvious bottleneck which was the listener de-registration slowness. >> >> I do worry however that any changes down the road which add listeners to the >> Node hierarchy again without fully understanding the implications would lead >> us to the same point we are now where the slowness of listener >> de-registrations becomes an issue again. There are no tests that catch this >> scenario. >> I feel that ideally both solutions are needed but am happy to bow to the >> more experienced OpenJFX devs opinions here as I know my changes may be more >> fundamental and hence risky. > > The problem is that there are usually many nodes, but only very few scenes > and windows, so registering a listener for each node on a scene or window is > pretty bad design (also consider the amount of notifications that a scene > change would trigger in such scenarios). As far as I can see, this is the > only such listener and only needed for two very limited cases, and its > addition in the past may have slipped through the cracks. > > Adding a performance unit test that specifically checks add/remove > performance of nodes may prevent such future regressions. @hjohn, agreed regards the issues of adding a listener to each node. Would it be worth doing the additional work of updating PopupWindow and ProgressIndicatorSkin to add their own listeners to make this a pull request that can be reviewed officially? I await any further comments from @kevinrushforth et al. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
