On Thu, 16 Apr 2020 16:15:20 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> @hjohn I have 12136 change listeners when debugging our application as you >> suggested. >> >> Please note that I see the issue when the TableView is having items added to >> it. If you just have a static TableView I >> do not see the issue. >> It is only when you add items to the TableView which causes a myriad of >> listeners to be deregistered and registered. >> The Visual VM snapshot I attached above was taken as our application was >> adding items to the TableView. > > I've tested this pull request locally a few times, and the performance > improvement is quite significant. A test with > 20.000 nested stack panes resulted in these average times: > - Add all 51 ms > - Remove all 25 ms > > Versus the unmodified code: > > - Add all 34 ms > - Remove all 135 ms > > However, there are some significant functional changes as well that might > impact current users: > > 1. The new code ensures that all listeners are notified even if the list is > modified during iteration by always making > a **copy** when an event is fired. The old code only did so when it was > **actually** modified during iteration. This > can be mitigated by making the copy in the code that modifies the list (as > the original did) using the `locked` flag to > check whether an iteration was in progress. 2. There is a significant > increase in memory use. Where before each > listener occupied an entry in an array, now each listener is wrapped by > `Map.Entry` (the Integer instance used per > entry can be disregarded). I estimate around 4-8x more heap will be consumed > (the numbers are small however, still > well below 1 MB for 20.000 entries). If this is an issue, a further level > could be introduced in the listener > implementation hierarchy (singleton -> array -> map). 3. Even though the > current version of this pull request takes > care to notify duplicate listeners the correct amount of times, it does not > notify them in the correct order with > respect to other listeners. If one registers listeners (a, b, a) the > notification order will be (a, a, b). The last > point is not easily solved and could potentially cause problems. Finally I > think this solution, although it performs > well is not the full solution. A doubling or quadrupling of nodes would > again run into serious limitations. I think > this commit > https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd > should not have introduced > another listener for each Node on the Window class. A better solution would > be to only have the Scene listen to Window > and have Scene provide a new combined status property that Node could use for > its purposes. Even better however would > be to change the properties involved to make use of the hierarchy naturally > present in Nodes, having child nodes listen > to their parent, and the top level node listen to the scene. This would > reduce the amount of listeners on a single > property in Scene and Window immensely, instead spreading those listeners > over the Node hierarchy, keeping listener > lists much shorter, which should scale a lot better. @hjon > 3. Even though the current version of this pull request takes care to notify > duplicate listeners the correct amount of > times, it does not notify them in the correct order with respect to other > listeners. If one registers listeners (a, b, > a) the notification order will be (a, a, b). Unless I'm missing something I don't think this is the case. I used a LinkedHashMap which preserved the order of notifications. Actually some unit tests failed if the notifications weren't carried out in the same order as registration which was the case when I used a HashMap. See here: https://github.com/openjdk/jfx/pull/108#issuecomment-590883183 ------------- PR: https://git.openjdk.java.net/jfx/pull/108
