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

[yosbits]

On Tue, 29 Dec 2020 09:50:42 GMT, Jose Pereda <jpereda at openjdk.org> wrote:

>> I commented.
>
> I've run SelectListView and SelectTableView tests and both run fine. As for the SelectTableView test, with 700_000 rows, when there is no selection, pressing SelectToEnd takes around 3.1 seconds, as expected. However, if there is one single row selected, after pressing SelectToEnd, the selection doesn't complete, and I have to abort and quit the app. 
> 
> Instead of:
> tableView.getSelectionModel().selectRange(tableView.getSelectionModel().getFocusedIndex(), tableView.getItems().size());
> this:
> int focusedIndex = tableView.getSelectionModel().getFocusedIndex();
> tableView.getSelectionModel().clearSelection();
> tableView.getSelectionModel().selectRange(focusedIndex, tableView.getItems().size());
> seems to work and times are again around 3 seconds or less.
> 
> Maybe you can check why that is happening?
>  
> And about the test discussion above, I understand that running the included tests as part of the automated test wouldn't make much sense (as these can take too long). However, maybe these could be added as unit tests with a reduced number of item/rows. Instead of measuring performance (selection times would depend on each machine), I'd say you could simply verify that selection works as expected.
> 
> While most of the changes are just about caching `size()`, the new implementation of `MultipleSelectionModelBase::indexOf` adds some new code that should be tested, as part of the unit tests, again not for performance, but to assert it returns the expected values.

As an overview, the new implementation can handle selectRange up to 50_000 records. This assumes general manual operation. However, after clearing the selection (a rare operation in manual operation), it is as efficient as selectAll. However, this is a two-time change.

The response difference is caused by the next conditional branch (size == 0) of SortedList. This will be the next hotspot to improve as seen by this improvement.

I can't include it in this PR because I don't have time to work.

I haven't tried it, but if I change the per-record insertToMapping call of this method to per-range setAllToMapping, the performance seems to be around selectAll.

javafx.collections.transformation.SortedList.java
 SortedList.java
    private void addRemove(Change<? extends E> c) {
        if (c.getFrom() == 0 && c.getRemovedSize() == size) {
            removeAllFromMapping();
        } else {
            for (int i = 0, sz = c.getRemovedSize(); i < sz; ++i) {
                removeFromMapping(c.getFrom(), c.getRemoved().get(i));
            }
        }
        if (size == 0) {
            setAllToMapping(c.getList(), c.getTo()); // This is basically equivalent to getAddedSubList
                                                     // as size is 0, only valid "from" is also 0
        } else {
            for (int i = c.getFrom(), to = c.getTo(); i < to; ++i) {
                insertToMapping(c.getList().get(i), i);
            }
        }
    }

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

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