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 preliminary testing looks good, thanks for adding tests! (will finish my review next week) modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java line 1761: > 1759: for (index++; index < minSize && > !prevState.get(index).equals(newState.get(index)); index++); > 1760: // The positions at [from, index) in prevState > were replaced by the corresponding positions in newState. > 1761: List<TablePosition<S, ?>> removed = > (List<TablePosition<S, ?>>) (List<?>) prevState.subList(from, index); `(List<TablePosition<S, ?>>) (List<?>)` generics gone wild! it's weird that this is required to make it compile. I wonder if it might be better to drop generics altogether: List removed = prevState.subList(from, index); var c = new NonIterableChange.GenericAddRemoveChange(from, index, removed, newState); sm.fireCustomSelectedCellsListChangeEvent(c); (we've lost type safety either way) ------------- PR Review: https://git.openjdk.org/jfx/pull/2131#pullrequestreview-4057460046 PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3034547929
