On Sun, 7 Dec 2025 01:36:02 GMT, Cormac Redmond <[email protected]> wrote:
> Fix SortedList memory leak by clearing unused entries in the internal array > when the source list shrinks. Also nullify tempElement.e after use, > > Adding > "--add-opens=javafx.base/javafx.collections.transformation=ALL-UNNAMED" to > allow reflection access from tests. This looks pretty good, I left a remark about how you clear the unused part of the list. modules/javafx.base/src/main/java/javafx/collections/transformation/SortedList.java line 284: > 282: // Already null, stop checking further > 283: break; > 284: } I think this is unsafe, depending on where the `null`s are sorted in the list (especially with a `nullsFirst` comparator). It may be better to get the `size` at the start of this method, and then clear anything from `size` to `oldSize` unconditionally. modules/javafx.base/src/test/java/test/javafx/collections/SortedListTest.java line 622: > 620: SortedList<String> unsortedSortedList = new SortedList<>(list); > 621: assertEquals(0, sortedList.size()); > 622: assertEquals(0, unsortedSortedList.size()); You could consider making the test parameterized. modules/javafx.base/src/test/java/test/javafx/collections/SortedListTest.java line 677: > 675: checkElementsBeyondSizeAreNull(1, > ReflectionUtils.getFieldValue(sortedList, "sorted")); > 676: checkElementsBeyondSizeAreNull(1, > ReflectionUtils.getFieldValue(unsortedSortedList, > 677: "sorted")); So I guess that `null`s are sorted towards the end? I think this will fail when they are sorted near the beginning (won't clear element `z` and `y`). ------------- PR Review: https://git.openjdk.org/jfx/pull/2000#pullrequestreview-3550364865 PR Review Comment: https://git.openjdk.org/jfx/pull/2000#discussion_r2597245820 PR Review Comment: https://git.openjdk.org/jfx/pull/2000#discussion_r2597241565 PR Review Comment: https://git.openjdk.org/jfx/pull/2000#discussion_r2597240243
