On Mon, 30 Mar 2026 15:48:49 GMT, Andy Goryachev <[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 > > Reviewers: @andy-goryachev-oracle > /reviewers 2 Reviewers: @andy-goryachev-oracle I also want and will review this in the next days :) > There are few certainties in life, but I am 100% certain they will never > upgrade to a newer version of JFX. I see. As far as I can tell, most backports will only hit the latest "LTS" version, meaning version 25 and maybe version 21. Since you mentioned that you're using JFX19, it's almost certainly not covered at the moment. But feel free to ask on the mailing list! ------------- PR Comment: https://git.openjdk.org/jfx/pull/2131#issuecomment-4161480046
