RFR: 8185886: Improve scrolling performance of TableView and TreeTableView

[dannygonzalez]

On Wed, 12 Feb 2020 12:22:15 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>>> hmm ... wouldn't the change violate spec of adding listeners:
>>> 
>>> > If the same listener is added more than once, then it will be notified more than once.
>> 
>> True, I hadn't read that spec in ObservableValueBase. 
>> Although that does seem odd behaviour to me. Obviously as the original implementation was using an array I can see how the implementation drove that specification.
>> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests all passed. It would be nice if there was a specific test case for this behaviour.
>> 
>> I would need to store a registration count for each listener to satisfy this requirement.
> 
>> 
>> Although that does seem odd behaviour to me. Obviously as the original implementation was using an array I can see how the implementation drove that specification.
>> 
> 
> whatever drove it (had been so since the beginning ot java desktop, at least since the days of swing), there is no way to change it, is it?
> 
>> Non of the JavaFx unit tests test for that specific case as the unit tests all passed. It would be nice if there was a specific test case for this behaviour.
>> 
> 
> yeah, the test coverage is ... not optimal :)
> 
>> I would need to store a registration count for each listener to satisfy this requirement.
> 
> a count plus some marker as to where it was added: 
> 
>    addListener(firstL)
>    addListener(secondL)
>    addListener(firstL)
> 
> must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. which brings us back to .. an array?

The listeners are called back in the order they were registered in my implementation although I didn’t see this requirement in the spec unless I missed something.

The performance unregistering thousands of listeners by TableView from the array is killing the performance of our JavaFX application. It takes up to 60% of JavaFX Application thread CPU and there are various bug reports around this same TableView performance issue.
The set implementation has lowered this to below 1%.

This pull request may be too fundamental a change to go into mainline. We however have to build our local OpenJFX with this fix or our application is unusable.

I’m happy to receive any suggestions.

Danny


On 12 Feb 2020, at 12:22, Jeanette Winzenburg <notifications at github.com<mailto:notifications at github.com>> wrote:


Although that does seem odd behaviour to me. Obviously as the original implementation was using an array I can see how the implementation drove that specification.

whatever drove it (had been so since the beginning ot java desktop, at least since the days of swing), there is no way to change it, is it?

Non of the JavaFx unit tests test for that specific case as the unit tests all passed. It would be nice if there was a specific test case for this behaviour.

yeah, the test coverage is ... not optimal :)

I would need to store a registration count for each listener to satisfy this requirement.

a count plus some marker as to where it was added:

addListener(firstL)
addListener(secondL)
addListener(firstL)

must result in firstL.invalidated, seconL.invalidated, firstL.invalidated .. which brings us back to .. an array?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://github.com/openjdk/jfx/pull/108?email_source=notifications&email_token=ABTEOIWBBLX3XA3JV23OP6LRCPSXRA5CNFSM4KQ7YBNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELQSLEY#issuecomment-585180563>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABTEOIVSPJEJ6FAJ5SFT7V3RCPSXRANCNFSM4KQ7YBNA>.

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

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