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