[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8197991: Selecting many items in a TableView is very slow [v3]
From:       yosbits <github.com+7517141+yososs () openjdk ! java ! net>
Date:       2020-12-30 8:41:55
Message-ID: TOz8dLRf2jmRNjiT43FIg7p-bLHA8Lb2FXnJbZfetik=.296eec57-58cd-4868-94d3-a99532ad6995 () github ! com
[Download RAW message or body]

On Tue, 29 Dec 2020 09:50:42 GMT, Jose Pereda <jpereda@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


[prev in list] [next in list] [prev in thread] [next in thread]