RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

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.

@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