On Thu, 14 May 2026 17:59:34 GMT, Andy Goryachev <[email protected]> wrote:
>> This PR fixes the (rare) issue that an `ArrayIndexOutOfBoundsException` is >> thrown in `FilteredList`. >> >> Some details of the problem: >> - Can only be reproduced when there are multiple changes, as we need an >> intermediate state where we have more elements than we will have at the end >> after all change events >> - The issue is somewhat rare: Normally, we have reserved a big enough array >> (`int[] filtered`) to compensate a temporary element increase. With a small >> size like 1, we can reproduce it easily by making two change events (In the >> reproducer and test, a 'move element to first index' change operation) >> >> Details for the fix: >> - `ensureSize(getSource().size());` is normally correct as our >> `FilteredList` will always be <= source list. >> But we can have the situation where we have temporarily more elements than >> the source list as we are processing all change events (e.g. add 1, remove >> 1). >> - To account for that, we will call `ensureSize` again when we could run >> into the situation where we have more elements than we reserved for >> - So that we do not over allocate the `filtered` array, I did not use the >> proposed change in the ticket but this one instead. We will really only >> (may) increase the size when we really need it >> - As we reserve more capacity in `filtered` than we may need, this does >> not happen often, especially not with more elements >> - Since we have a smart change event aggregation algorithm (especially >> since the fix in https://github.com/openjdk/jfx/pull/2154 and >> https://github.com/openjdk/jfx/pull/2117), we often have more compact change >> event where a big increase of elements temporary, which are removed in the >> next change are very unlikely (there will be only one change event with the >> elements that 'survived' the add and remove). >> - Therefore, I do not see any performance concerns here, as >> `ensureSize(size + 1);` is very likely a noop, but in cases like this e will >> correctly reserve more capacity >> >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > modules/javafx.base/src/main/java/javafx/collections/transformation/FilteredList.java > line 138: > >> 136: @Override >> 137: protected void sourceChanged(Change<? extends E> c) { >> 138: ensureSize(getSource().size()); > > are you sure this is needed? > > the tests in the two JBS tickets pass without it, `addRemove()` invokes this > method in ~L239 (then calls it again with size+1 in L273). `update()` calls > it in ~L286, and `permutate()` does not need it. > > perhaps `addRemove()` can be further optimized so as not to call > `ensureSize()` twice? `update()` and `permutate()` do not call it anymore. I relocated them into one single location: Here. So functionality wise, it did not change. We could probably remove it, as we will catch any grow on size in `addRemove()`. But it might hurt performance. If we have a `FilteredList` with a size 1 and add 10_000 elements, we will call `ensureSize` a lot of times, which will lead to a degraded performance. But since we call it once here, we will immediately grow to the worst case size - if nothing need to be filtered and MAY only grow it in `addRemove()` when we temporarily have more elements due to processing the changes one by one. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2163#discussion_r3261319763
