RFR: 8185886: Improve scrolling performance of TableView and TreeTableView
github.com+6702882+dannygonzalez at openjdk.java.net
Fri Apr 17 07:10:24 UTC 2020
On Thu, 16 Apr 2020 16:15:20 GMT, John Hendrikx <jhendrikx at 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.
> 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:
More information about the openjfx-dev