On Thu, 16 Apr 2020 09:45:20 GMT, dannygonzalez <github.com+6702882+dannygonza...@openjdk.org> wrote:
>> @dannygonzalez Could you perhaps debug your application and take a look at >> how large the following array is: a random node -> `scene` -> `value` -> >> `window` -> `readOnlyProperty` -> `helper` -> `changeListeners`. I just >> tested this with a custom control displaying 200 cells on screen at once >> (each cell consisting of about 30 nodes itself), and I saw about 20000 >> change listeners registered on this single Scene Window property. >> >> However, this custom control is not creating/destroying cells beyond the >> initial allocation, so there wouldn't be any registering and unregistering >> going on, scrolling was still smooth >30 fps. > > @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. ------------- PR: https://git.openjdk.java.net/jfx/pull/108
