On Wed, 1 Apr 2026 02:22:21 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 > > Charlie Schlosser has updated the pull request incrementally with one > additional commit since the last revision: > > fix typo in comments modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 1753: > 1751: for (int index = 0; index < minSize; index++) { > 1752: // Advance to the next position where prevState > and newState are not equal. > 1753: for (; index < minSize && > prevState.get(index).equals(newState.get(index)); index++); One thing I noted here and below: We probably should guard against nullpointer. So we could change that to: Objects.equals(prevState.get(index), newState.get(index)) and we are safe for whatever state might be in there. modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 1759: > 1757: } > 1758: // Advance to the next position where prevState > and newState are equal. > 1759: for (index++; index < minSize && > !prevState.get(index).equals(newState.get(index)); index++); !Objects.equals(prevState.get(index), newState.get(index)) modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 1769: > 1767: // The positions at [minSize, prevSize) in > prevState were removed. If prevSize == minSize, the range is empty. > 1768: // The positions at [minSize, newSize) in > newState were added. If newSize == minSize, the range is empty. > 1769: List<TablePosition<S, ?>> removed = > (List<TablePosition<S, ?>>) (List<?>) prevState.subList(minSize, prevSize); If `minSize == prevSize`, we could save the `subList` call here and just use `List.of()`. Will save a bit, as `subList` will always allocate a new, well, sub `List`. (Even if it is empty). modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 4051: > 4049: assertEquals(List.of(0, 3, 6), froms); > 4050: assertEquals(List.of(2, 5, 8), tos); > 4051: assertEquals(List.of(0, 1), > removedLists.get(0).stream().map(TablePosition::getRow).toList()); Very minor: Here and below are some unneeded whitespaces modules/javafx.controls/src/test/java/test/javafx/scene/control/TableViewTest.java line 4106: > 4104: assertEquals(List.of(1, 2), > removedLists.get(0).stream().map(TablePosition::getRow).toList()); > 4105: > 4106: froms.clear(); Minor: I think this could be easily a new test. Then you don't need to clear the variables and the test is a bit shorter. And makes sense since this is another scenario ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3039396842 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3039397386 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3039387926 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3039409399 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3039408020
