RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[Nir Lisker]

On Mon, 2 Mar 2020 08:31:47 GMT, dannygonzalez <github.com+6702882+dannygonzalez at openjdk.org> wrote:

>> In use cases where a large number of listeners are being discarded, it may be better to first consider changing the design to receive event notifications on nodes whose listener registrations are not frequently discarded.
>
>> Hmm .. personally, I would consider changing such a basic (and probably highly optimized) implementation used all over the framework is a very high risk change that at the worst outcome would detoriate internal and external code. So before going that lane, I would try - as you probably have, just me not seeing it :) - to tackle the problem from the other end:
>> 
>> > I know that in our application, the **thousands of listeners** unregistering when using a TableView was stalling the JavaFX thread.
>> 
>> .. sounds like a lot. Any idea, where exactly they come into play?
> 
> I did start to look at why there were so many change listeners unregistering but felt that would take a deeper understanding of the architecture and design decisions of JavaFX scene graph and I didn't have that time to invest. 
> I do know that there are thousands of them unregistering in a TableView and unregistering them is a bottleneck for the JavaFX thread.
> 
> There are multiple change listeners on a Node for example, so you can imagine with the number of nodes in a TableView this is going to be a problem if the unregistering is slow.
> 
> To get our application usable I profiled the code and saw this unregistering as a major bottleneck, hence why I looked at this more obvious solution.
> 
> I'm happy to look at the problem from the other angle and happy to listen to suggestions from people with more experience of the design and architecture but tackling the problem from the perspective of re-architecting the behaviour of listeners in the scene graph would be more work than I could feasibly take on.

> technically true - but the implementation was linear with a fixed sequence since-the-beginning-of-java-desktop-time (and sometimes, for good ol' beans properties, even exposed as api to access the array of listeners). So technically, we could go the path of explicitely spec'ing that the order is unspecified. Pretty sure that doing so and implementing it will break tons of application code that's subtly relying on fifo notification (f.i. register before or after skin has its own is a simple wide-spread trick) .. which all did it wrong and might deserve it ;-) But then if even internal code does it ..

So we can choose to specify it as ordered.

These are the 2 options I mentioned:
1. Not specify the behavior and change the implementation in favor of performance. This could break applications as you've mentioned.
2. Specify that the order is preserved and keep the ordered implementation behavior. This will allow applications to rely on the behavior safely.

Right now we're losing on both sides. We keep a promise and we don't tell anyone about it. The only reason for this is if we intend to change the behavior in the future, in which case we should add a doc comment saying that the order is unspecified and is planned to change in the future, so there will be at least some warning.

Once we choose what to do here it will make sense to continue to review the code with that decision in mind.

-------------

PR: https://git.openjdk.java.net/jfx/pull/108