On Wed, 15 Apr 2020 07:11:27 GMT, dannygonzalez <github.com+6702882+dannygonza...@openjdk.org> wrote:
>> @dannygonzalez You mentioned "There are multiple change listeners on a Node >> for example, so you can imagine with the number of nodes in a TableView this >> is going to be a problem if the unregistering is slow.". >> >> Your changes in this PR seem to be focused on improving performance of >> unregistering listeners when there are thousands of listeners listening on >> changes or invalidations of the **same** property. Is this actually the >> case? Is there a single property in TableView or TreeTableView where such a >> high amount of listeners are present? If so, I think the problem should be >> solved there. >> >> As it is, I donot think this PR will help unregistration performance of >> listeners when the listeners are spread out over dozens of properties, and >> although high in total number, the total listeners per property would be low >> and their listener lists would be short. Performance of short lists (<50 >> entries) is almost unbeatable, so it is relevant for this PR to know if >> there are any properties with long listener lists. > > @hjohn > I haven't quantified exactly where all the listeners are coming from but the > Node class for example has various listeners on it. > > The following changeset (which added an extra listener on the Node class) > impacted TableView performance significantly (although it was still bad > before this change in my previous tests): > >> commit e21606d3a1b73cd4b44383babc750a4b4721edfd >> Author: Florian Kirmaier <fk at sandec.de<mailto:fk at sandec.de>> >> Date: Tue Jan 22 09:08:36 2019 -0800 >> >> 8216377: Fix memoryleak for initial nodes of Window >> 8207837: Indeterminate ProgressBar does not animate if content is added >> after scene is set on window >> >> Add or remove the windowShowingChangedListener when the scene changes > > As you can imagine, a TableView with many columns and rows can be made up of > many Node classes. The impact of having multiple listeners deregistering on > the Node when new rows are being added to a TableView can be a significant > performance hit on the JavaFX thread. > > I don't have the time or knowledge to investigate why these listeners are all > needed especially around the Node class. I went directly to the source of the > problem which was the linear search to deregister each listener. > > I have been running my proposed fix in our JavaFX application for the past > few months and it has saved it from being unusable due to the JavaFx thread > being swamped. The patch proposed here does not share the case where the listener deletion performance becomes a bottleneck. I think that it is necessary to reproduce it by testing first, but If you just focus on improving listener removal performance, If the lifespan of a large number of registered listeners is biased, It seems like the next simple change can improve delete performance without changing the data structure. * Change the search from the front of the list to the search from the back. This will reduce the number of long-life listeners matching. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
