On Fri, 26 Nov 2021 19:49:37 GMT, Abhinay Agarwal <d...@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
