RFR: 8197991: Selecting many items in a TableView is very slow [v3]

[Kevin Rushforth]

On Fri, 26 Nov 2021 19:49:37 GMT, Abhinay Agarwal <duke at openjdk.java.net> wrote:

>> 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](https://github.com/openjdk/jfx/pull/127) submitted by @yososs
>> 
>> There are currently 2 manual tests for this fix.
>
> Abhinay Agarwal has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update ROW_COUNT to 700_000

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.

modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java line 877:

> 875:             if (size >= 0) {
> 876:                 return size;
> 877:             }

Using lazy evaluation means that you need to be extra careful that the size is invalidated in all the right places. One method that needs to be checked is the `set(int index, int... indices)` method. How carefully have you checked to make sure that nothing that could change the size fails to update the `size` field?

Related to this, are you satisfied that the current set of unit tests are sufficient to catch any potential problems, and that you don't need to add more tests?

modules/javafx.controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java line 880:

> 878:         @Override public int indexOf(Object obj) {
> 879:             reset();
> 880:             return super.indexOf(obj);

So `reset` doesn't need to still be called?

tests/manual/controls/SelectListViewTest.java line 1:

> 1: import javafx.application.Application;

This needs a copyright header.

tests/manual/controls/SelectListViewTest.java line 24:

> 22: 
> 23:         ObservableList<String> items = listView.getItems();
> 24:         for(int i=0; i<ROW_COUNT; i++) {

Minor: add a space after `for` and spaces around the `=` and `<` operators.

tests/manual/controls/SelectListViewTest.java line 47:

> 45:         stage.setScene(new Scene(root, 600, 600));
> 46: 
> 47:         selectAll.setOnAction((e)->selectAll(listView));

Minor: add space before and after the `->` operator.

tests/manual/controls/SelectListViewTest.java line 66:

> 64:         long t = System.currentTimeMillis();
> 65:         listView.getSelectionModel().clearSelection();
> 66:         System.out.println("time:"+ (System.currentTimeMillis() - t));

MInor: space before the `+` here and a few places below.

tests/manual/controls/SelectTableViewTest.java line 1:

> 1: import javafx.application.Application;

This needs a copyright header.

tests/manual/controls/SelectTableViewTest.java line 28:

> 26: 
> 27:         final ObservableList<TableColumn<String[], ?>> columns = tableView.getColumns();
> 28:         for(int i=0; i<COL_COUNT; i++) {

Minor: add a space after `for` and spaces around the `=` and `<` operators.

tests/manual/controls/SelectTableViewTest.java line 37:

> 35: 
> 36:         ObservableList<String[]> items = tableView.getItems();
> 37:         for(int i=0; i<ROW_COUNT; i++) {

Minor: add a space after `for` and spaces around the `=` and `<` operators.

tests/manual/controls/SelectTableViewTest.java line 39:

> 37:         for(int i=0; i<ROW_COUNT; i++) {
> 38:             String[] rec = new String[COL_COUNT];
> 39:             for(int j=0; j<rec.length; j++) {

Minor: add a space after `for` and spaces around the `=` and `<` operators.

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

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