8197991: Selecting many items in a TableView is very slow #673

abhinayagarwal · 2021-11-17T05:34:46Z

This work improves the performance of MultipleSelectionModel over large data sets by caching some values and avoiding unnecessary calls to SelectedIndicesList#size. It further improves the performance by reducing the number of iterations required to find the index of an element in the BitSet.

The work is based on an abandoned patch submitted by @yososs

There are currently 2 manual tests for this fix.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8197991: Selecting many items in a TableView is very slow

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Ajit Ghaisas (@aghaisas - Reviewer)

Contributors

Naohiro Yoshimoto <yosbits@gmail.com>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/673/head:pull/673
$ git checkout pull/673

Update a local copy of the PR:
$ git checkout pull/673
$ git pull https://git.openjdk.java.net/jfx pull/673/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 673

View PR using the GUI difftool:
$ git pr show -t 673

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/673.diff

bridgekeeper · 2021-11-17T05:36:13Z

👋 Welcome back abhinayagarwal! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2021-11-17T08:59:53Z

Webrevs

kevinrushforth · 2021-11-18T00:27:32Z

I closed #127 on which this PR is based.

@abhinayagarwal Please use the /contributor command to add @yososs as a contributor.

/reviewers 2

openjdk · 2021-11-18T00:29:08Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

...vafx.controls/src/main/java/com/sun/javafx/scene/control/ReadOnlyUnbackedObservableList.java

Maran23 · 2021-11-18T08:53:07Z

Just wondering, isn't it also possible to write some unit tests for the MultipleSelectionModel(Base) ?

abhinayagarwal · 2021-11-22T14:27:22Z

/contributor add @yososs

openjdk · 2021-11-22T14:28:28Z

@abhinayagarwal Could not parse @yososs as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

abhinayagarwal · 2021-11-22T16:07:37Z

@Maran23 I don't know what would be an apt unit test as the PR changes implementation detail. If you have suggestions, please let me know.

There are 276 unit tests in test.javafx.scene.control.MultipleSelectionModelImplTest and all of them (excluding 4 ignored test cases) still pass with the changes made to this PR.

kevinrushforth · 2021-11-23T17:40:29Z

I see that the /contributor command didn't work with the original contributor's GitHub username. You can instead take the contributor information from the HEAD commit of the pull request: Naohiro Yoshimoto yosbits@gmail.com.

abhinayagarwal · 2021-11-24T02:21:28Z

/contributor add Naohiro Yoshimoto yosbits@gmail.com

openjdk · 2021-11-24T02:22:39Z

@abhinayagarwal
Contributor Naohiro Yoshimoto <yosbits@gmail.com> successfully added.

tests/manual/controls/SelectListViewTest.java

tests/manual/controls/SelectTableViewTest.java

abhinayagarwal · 2021-12-16T03:23:36Z

@kevinrushforth Is there something I can do to move this PR forward?

kevinrushforth

The fix looks good to me. I did leave a couple questions about the implementation.

The new manual tests need a copyright header, and I noted a few of the code formatting issues.

...vafx.controls/src/main/java/com/sun/javafx/scene/control/ReadOnlyUnbackedObservableList.java

modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java

tests/manual/controls/SelectListViewTest.java

tests/manual/controls/SelectTableViewTest.java

tests/manual/controls/SelectListViewTest.java

tests/manual/controls/SelectTableViewTest.java

tests/manual/controls/SelectListViewTest.java

kevinrushforth · 2021-12-17T22:15:15Z

Thanks for adding the manual test. I can confirm the performance gains. I ran the ListView test, clicked on one of the cells visible without scrolling, then pressed the "select to the end" operation. With the 70k cells defined by the test program that operation ran 13,715 times faster on my machine with this fix.

kevinrushforth

I think this is very close to being ready to go in. I left one more inline comment about the invalidation of size. There is also a pending minor code formatting comment.

modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java

kevinrushforth

The fix looks good. I left a few comments on the tests.

.../javafx.controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java

kevinrushforth · 2022-01-06T21:02:35Z

tests/manual/controls/SelectListViewTest.java

+    private void clearSelection(ListView listView) {
+        long t = System.currentTimeMillis();
+        listView.getSelectionModel().clearSelection();
+        System.out.println("time:"+ (System.currentTimeMillis() - t));


This is still pending.

tests/manual/controls/SelectTableViewTest.java

kevinrushforth

Looks good.

aghaisas

The code changes look good.
I tested on Mac. This PR drastically improves the selection time for the provided manual test.

openjdk · 2022-01-10T12:07:54Z

@abhinayagarwal This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8197991: Selecting many items in a TableView is very slow

Co-authored-by: Naohiro Yoshimoto <yosbits@gmail.com>
Reviewed-by: kcr, aghaisas

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 35 new commits pushed to the master branch:

be3b3bd: 8279328: CssParser uses default charset instead of UTF-8
487e4b1: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces
32f21ff: 8234921: Add DirectionalLight to the selection of 3D light types
1feba1f: 8279396: Define version in .jcheck/conf
c705bd4: 8203463: [Accessibility, Narrator] NPE in TableView
303bcdb: 8279078: Update copyright header for files modified in 2021
1f10c63: 8273096: Add support for H.265/HEVC to JavaFX Media
18063ad: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes
5422a5a: 8278860: Streamline properties for Monocle
4c5bf44: 8278905: JavaFX: EnumConverter has a typo in the toString method
... and 25 more: https://git.openjdk.java.net/jfx/compare/f939d094e3f02575c77a12c3a986d5f601a65a1d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @aghaisas) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

abhinayagarwal · 2022-01-11T04:42:04Z

/integrate

openjdk · 2022-01-11T04:42:49Z

@abhinayagarwal
Your change (at version 30fb31c) is now ready to be sponsored by a Committer.

nlisker · 2022-01-11T04:44:08Z

/sponsor

openjdk · 2022-01-11T04:44:59Z

Going to push as commit 8c4f966.
Since your change was applied there have been 35 commits pushed to the master branch:

be3b3bd: 8279328: CssParser uses default charset instead of UTF-8
487e4b1: 8267059: Gradle :clean and :apps tasks fail on Windows if ANT_HOME contains spaces
32f21ff: 8234921: Add DirectionalLight to the selection of 3D light types
1feba1f: 8279396: Define version in .jcheck/conf
c705bd4: 8203463: [Accessibility, Narrator] NPE in TableView
1f10c63: 8273096: Add support for H.265/HEVC to JavaFX Media
18063ad: 8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes
5422a5a: 8278860: Streamline properties for Monocle
4c5bf44: 8278905: JavaFX: EnumConverter has a typo in the toString method
... and 25 more: https://git.openjdk.java.net/jfx/compare/f939d094e3f02575c77a12c3a986d5f601a65a1d...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-11T04:45:20Z

@nlisker @abhinayagarwal Pushed as commit 8c4f966.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

abhinayagarwal and others added 2 commits November 17, 2021 10:42

8197991: Selecting many items in a TableView is very slow

145c0aa

Add manual tests

0d99eaf

abhinayagarwal added 2 commits November 17, 2021 13:37

Update line ending

ae4e01c

Update line ending for SelectTableViewTest

05f5df3

openjdk bot added the rfr Ready for review label Nov 17, 2021

kevinrushforth requested review from kevinrushforth and aghaisas November 18, 2021 00:23

kevinrushforth mentioned this pull request Nov 18, 2021

8197991: Selecting many items in a TableView is very slow #127

Closed

3 tasks

nlisker reviewed Nov 18, 2021

View reviewed changes

...vafx.controls/src/main/java/com/sun/javafx/scene/control/ReadOnlyUnbackedObservableList.java Show resolved Hide resolved

...vafx.controls/src/main/java/com/sun/javafx/scene/control/ReadOnlyUnbackedObservableList.java Show resolved Hide resolved

yososs reviewed Nov 26, 2021

View reviewed changes

tests/manual/controls/SelectListViewTest.java Show resolved Hide resolved

yososs reviewed Nov 26, 2021

View reviewed changes

tests/manual/controls/SelectTableViewTest.java Outdated Show resolved Hide resolved

abhinayagarwal added 2 commits November 27, 2021 00:58

Remove duplicate entry for test data

3158acd

Update ROW_COUNT to 700_000

8196b34

kevinrushforth reviewed Dec 17, 2021

View reviewed changes

abhinayagarwal added 2 commits December 21, 2021 17:46

Add license header and add space around operators

c7b2208

Add space around operator and string

2d32108

kevinrushforth reviewed Dec 22, 2021

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java Show resolved Hide resolved

abhinayagarwal added 2 commits January 6, 2022 13:40

Add tests

7a6af2a

Remove commented out method. Document constructors.

741aa92

kevinrushforth self-requested a review January 6, 2022 18:27

kevinrushforth reviewed Jan 6, 2022

View reviewed changes

abhinayagarwal added 2 commits January 7, 2022 13:28

Add size assertion to test

5ff9510

Format code and reduce test size

30fb31c

kevinrushforth approved these changes Jan 7, 2022

View reviewed changes

aghaisas approved these changes Jan 10, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jan 10, 2022

openjdk bot added the sponsor Ready to sponsor label Jan 11, 2022

openjdk bot added the integrated Pull request has been integrated label Jan 11, 2022

openjdk bot closed this Jan 11, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Jan 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8197991: Selecting many items in a TableView is very slow #673

8197991: Selecting many items in a TableView is very slow #673

abhinayagarwal commented Nov 17, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 17, 2021

mlbridge bot commented Nov 17, 2021 •

edited

Loading

kevinrushforth commented Nov 18, 2021

openjdk bot commented Nov 18, 2021

Maran23 commented Nov 18, 2021

abhinayagarwal commented Nov 22, 2021

openjdk bot commented Nov 22, 2021

abhinayagarwal commented Nov 22, 2021 •

edited

Loading

kevinrushforth commented Nov 23, 2021

abhinayagarwal commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

abhinayagarwal commented Dec 16, 2021

kevinrushforth left a comment

kevinrushforth commented Dec 17, 2021

kevinrushforth left a comment

kevinrushforth left a comment

kevinrushforth Jan 6, 2022

kevinrushforth left a comment

aghaisas left a comment

openjdk bot commented Jan 10, 2022 •

edited

Loading

abhinayagarwal commented Jan 11, 2022

openjdk bot commented Jan 11, 2022

nlisker commented Jan 11, 2022

openjdk bot commented Jan 11, 2022

openjdk bot commented Jan 11, 2022

8197991: Selecting many items in a TableView is very slow #673

8197991: Selecting many items in a TableView is very slow #673

Conversation

abhinayagarwal commented Nov 17, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Nov 17, 2021

mlbridge bot commented Nov 17, 2021 • edited Loading

Webrevs

kevinrushforth commented Nov 18, 2021

openjdk bot commented Nov 18, 2021

Maran23 commented Nov 18, 2021

abhinayagarwal commented Nov 22, 2021

openjdk bot commented Nov 22, 2021

abhinayagarwal commented Nov 22, 2021 • edited Loading

kevinrushforth commented Nov 23, 2021

abhinayagarwal commented Nov 24, 2021

openjdk bot commented Nov 24, 2021

abhinayagarwal commented Dec 16, 2021

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth commented Dec 17, 2021

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Jan 6, 2022

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

aghaisas left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 10, 2022 • edited Loading

abhinayagarwal commented Jan 11, 2022

openjdk bot commented Jan 11, 2022

nlisker commented Jan 11, 2022

openjdk bot commented Jan 11, 2022

openjdk bot commented Jan 11, 2022

abhinayagarwal commented Nov 17, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 17, 2021 •

edited

Loading

abhinayagarwal commented Nov 22, 2021 •

edited

Loading

openjdk bot commented Jan 10, 2022 •

edited

Loading