RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[John Hendrikx]

On Fri, 17 Apr 2020 08:48:35 GMT, dannygonzalez <github.com+6702882+dannygonzalez at openjdk.org> wrote:

>> @dannygonzalez I added a proof of concept here if you want to play with it: https://github.com/openjdk/jfx/pull/185
>
> @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.

-------------

PR: https://git.openjdk.java.net/jfx/pull/108