On Sun, 29 Mar 2026 04:10:35 GMT, Charlie Schlosser <[email protected]> wrote:
> Fixes [8380933](https://bugs.openjdk.org/browse/JDK-8380933) > > `TableView` and `TreeTableView` notify listeners when the selected cells > change after a sort with a very expensive check: > > > if (!newState.contains(prevItem)) { > removed.add(prevItem); > } > > > If `removed` is not empty, the method conservatively notifies the listeners > that the entire table was replaced: > > > ListChangeListener.Change<TreeTablePosition<S, ?>> c = new > NonIterableChange.GenericAddRemoveChange<>(0, itemCount, removed, newState); > > > The slowness is not attributed to the sort at all, but the logic in handling > change notifications. We can preserve this behavior and address the > performance issues with minimal changes by creating a temporary `HashSet` for > the `contains` checks. > > However, this change notification is conservative and includes cells that > weren't affected by the sort. For example: > > before sort: `{"a", "c", "b"}` with selected cells `{"0", "1"}` > > after sort: `{"a", "b", "c"}` with selected cells `{"0", "2"}` > > Although the first item is not affected by the sort, the current code will > notify listeners that cells `{"0", "2"}` were added and `{"1"}` was removed. > That is, the entire selection set was replaced. > > This PR scans `prevState` and `newState` and fires change events for the > affected ranges only, which addresses the performance issues, and minimizes > notifications. This PR also handles the scenario where the size of the > selection set is changed after the sort. This seemed prudent as a custom sort > policy could do anything to the selection set. > > Some benchmarks with a `TableView` of integers with `selectAll`, illustrating > that the performance issue had nothing to do with the sort, and was entirely > attributed to the `contains` loop. > > Input Size | Old Time (ms) | New Time (ms) > -- | -- | -- > 64 | 1 | 1 > 128 | 0 | 0 > 256 | 0 | 0 > 512 | 1 | 0 > 1024 | 3 | 0 > 2048 | 4 | 1 > 4096 | 14 | 0 > 8192 | 47 | 1 > 16384 | 186 | 1 > 32768 | 844 | 1 > 65536 | 3603 | 2 > 131,072 | 16,096 | 2 Some initial comments. They also apply for `TreeTableView` and the corresponding tests modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 1751: > 1749: int newSize = newState.size(); > 1750: int minSize = Math.min(prevSize, newSize); > 1751: for (int i = 0; i < minSize; i++) { Maybe name `i` to `index` to make it more clear. Or `cellIndex` or something like that. modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 1753: > 1751: for (int i = 0; i < minSize; i++) { > 1752: while (i < minSize && > prevState.get(i).equals(newState.get(i))) i++; > 1753: if (i >= minSize) break; Please always use `{}` in while and if statements. Makes the code more readable and is usually the code style we use modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 1762: > 1760: } > 1761: if (prevSize != newSize) { > 1762: // tail was purely removed (prevSize > newSize) > or purely added (prevSize < newSize) I'm not sure I understand the comment. What does tail mean, in what context? So I would prefer more clear comments, even if they are multiple lines long. As this code is not super trivial to understand, and we will likely be reading this code many times in the coming years. modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 4018: > 4016: > 4017: TableView<String> table = new TableView<>(); > 4018: table.setItems(FXCollections.observableArrayList("b", "c", > "a")); The only thing I'm a bit afraid in this and the other test: We use a very small amount of items. I guess it is fine, but I still wonder if we should use a bit more. To be safe. modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 4034: > 4032: }); > 4033: > 4034: StageLoader sl = new StageLoader(table); I would recommend using `stageLoader = new StageLoader(table);`. Then you can also remove the `sl.dispose();` below. modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 4049: > 4047: } > 4048: > 4049: @Test public void > testSortFiresRemovalEventForSelectionSizeMismatch() { Minor: `public` is not needed for JUnit 5 Tests, I would always recommend to remove as this recommended by the JUnit Team as well. modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 4062: > 4060: table.setSortPolicy(tv -> { > 4061: FXCollections.sort(tv.getItems(), > Comparator.naturalOrder()); > 4062: tv.getSelectionModel().clearSelection(1); I think a test where we select some values would also be good. What do you think? ------------- Changes requested by mhanl (Committer). PR Review: https://git.openjdk.org/jfx/pull/2131#pullrequestreview-4027109105 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3006590428 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3006591570 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3006594478 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3006604699 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3006600725 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3006602479 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3006602873
