RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic

[Kevin Rushforth]

On Wed, 12 Feb 2020 12:43:58 GMT, dannygonzalez <github.com+6702882+dannygonzalez at openjdk.org> 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?
>
> 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>.

@dannygonzalez the reason for the `jcheck` failure is that you have commits with two different email addresses in your branch. At this point, it's probably best to squash the commits into a single commit with `git rebase -i master` (presuming that your local `master` is up to date), and then do a force-push.

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

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