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