RFR: 8252936: Optimize removal of listeners from ExpressionHelper.Generic [v3]

[John Hendrikx]

On Wed, 14 Apr 2021 12:33:23 GMT, dannygonzalez <github.com+6702882+dannygonzalez at openjdk.org> wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8185886
>> 
>> Optimisation to ExpressionHelper.Generic class to use Sets rather than Arrays to improve listener removal speed.
>> 
>> This was due to the removal of listeners in TableView taking up to 50% of CPU time on the JavaFX Application thread when scrolling/adding rows to TableViews.
>> 
>> This may alleviate some of the issues seen here:
>> 
>> TableView has a horrific performance with many columns #409
>> https://github.com/javafxports/openjdk-jfx/issues/409#event-2206515033
>> 
>> JDK-8088394 : Huge memory consumption in TableView with too many columns
>> JDK-8166956: JavaFX TreeTableView slow scroll performance
>> JDK-8185887: TableRowSkinBase fails to correctly virtualise cells in horizontal direction
>> 
>> OpenJFX mailing list thread: TableView slow vertical scrolling with 300+ columns
>> https://mail.openjdk.java.net/pipermail/openjfx-dev/2020-January/024780.html
>
> dannygonzalez has updated the pull request incrementally.

I think it is also important to check if this is still an issue.

A PR which reduces the amount of listeners on Scene/Window was 
integrated in JavaFX 17: https://github.com/openjdk/jfx/pull/185

Are you experiencing problems still under 17?

I think PR 108 is addressing symptoms of a problem but not the root 
cause -- is it ever useful to have >10000 listeners on a single property 
which would warrant the use of Sets?  PR 185 solved this by not 
registering a listener for each Node on the Scene and Window, and there 
was some confirmation from the OP that this also addressed the issue, 
see here: https://github.com/openjdk/jfx/pull/108#issuecomment-615125904

--John

On 01/10/2021 13:27, John Nader wrote:
> This change is very near completion. According to @yososs
> <https://github.com/yososs>, what remains is to split the work into two
> separate PRs. The first PR must contain the updated test covering
> additional cases to ensure backwards compatibility. The second should
> contain the performance enhancement changes. This will allow the tests
> to be merged into master first, to ensure the behavior is maintained
> before and after the performance enhancement.
>
> I see that @dannygonzalez <https://github.com/dannygonzalez> has not
> been able to continue this work. If possible and appropriate, I would
> like to finish this work. I have already setup a build environment and
> have verified that the updated tests alone pass when integrated with the
> master branch and have 100% coverage of the modified class.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/openjdk/jfx/pull/108#issuecomment-932146935>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAHTETIDTFZAQSKBVUFO3BLUEWLI7ANCNFSM4KQ7YBNA>.
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>

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

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