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 @hjohn > 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). There was discussion about lazy initialisation of the LinkedHashMap when needed and/or have some sort of strategy where we could use arrays or lists if the number of listeners are less than some threshold (i.e. introducing another level to the hierarchy as you mentioned). This was mentioned here also: https://github.com/openjdk/jfx/pull/108#issuecomment-590838942 ------------- PR: https://git.openjdk.java.net/jfx/pull/108
