On Thu, 6 Jan 2022 08:17:53 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 two > additional commits since the last revision: > > - Remove commented out method. Document constructors. > - Add tests The fix looks good. I left a few comments on the tests. modules/javafx.controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java line 1427: > 1425: assertTrue(model.isSelected(2)); > 1426: assertTrue(model.isSelected(5)); > 1427: assertFalse(model.isSelected(0)); I recommend to also check the size. modules/javafx.controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java line 1436: > 1434: model.clearSelection(); > 1435: > 1436: assertTrue(model.getSelectedIndices().isEmpty()); I recommend to also check the size. tests/manual/controls/SelectTableViewTest.java line 40: > 38: public class SelectTableViewTest extends Application { > 39: > 40: final int ROW_COUNT = 700_000; This count is too large, even with the fix. I recommend changing it to something smaller (no more than `70_000`, which matches what you did for `SelectListViewTest`). tests/manual/controls/SelectTableViewTest.java line 101: > 99: long t = System.currentTimeMillis(); > 100: tableView.getSelectionModel().selectAll(); > 101: System.out.println("time:"+ (System.currentTimeMillis() - t)); Minor: space before the `+` here and a few places below (also in the other test as noted). ------------- PR: https://git.openjdk.java.net/jfx/pull/673
