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. 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
