On Sun, 29 Mar 2026 18:33:06 GMT, Marius Hanl <[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 > > 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. - `prevSize > newSize` : the elements `[newSize, prevSize)` in `prevState` were removed - `prevSize < newSize` : the elements `[prevSize, newSize)` in `newState` were added Both cases are handled when `from = minSize`, `to = newSize`, and `removed = prevState.subList(minSize, prevSize)` ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2131#discussion_r3006762898
