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

[dannygonzalez]

On Mon, 17 Feb 2020 13:51:49 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> 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.

@kevinrushforth just a note to say there are other ExpressionHelper classes (i.e. MapExpressionHelper, SetExpressionHelper and ListExpressionHelper) that also use arrays and suffer from the linear search issue when removing listeners. 

These however didn't appear in the critical path of the JavaFXThread and didn't come up in my profiling of TableView.

If this pull request is accepted, my opinion is that they should probably all move to using the same pattern as here, which is to use Maps instead of Arrays for their listener lists so that all these classes are uniform.

Thanks

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

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