Skip to content

8195614: FilteredList throws ArrayIndexOutOfBoundsException if created with 1 element #2163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Maran23 wants to merge 1 commit into
base: master
Choose a base branch
from
+21 −3
Open

8195614: FilteredList throws ArrayIndexOutOfBoundsException if created with 1 element #2163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions modules/javafx.base/src/main/java/javafx/collections/transformation/FilteredList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2026, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -135,6 +135,8 @@ private Predicate<? super E> getPredicateImpl() {

@Override
protected void sourceChanged(Change<? extends E> c) {
ensureSize(getSource().size());
Copy link
Copy Markdown
Contributor

@andy-goryachev-oracle andy-goryachev-oracle May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

@Maran23 Maran23 May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@andy-goryachev-oracle andy-goryachev-oracle May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misunderstanding something.

  1. Is ensureSize() L138 needed for permutate() ?
  2. addRemove() calls ensureSize() twice (L138 and L237). can it call it only once?
  3. update() does not change the filtered size, right? why is it calling ensureSize() in L138?

Copy link
Copy Markdown
Member Author

@Maran23 Maran23 May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. No - but when we are only sorting (permutation) the list, the ensureSize should be a noop, since the size had to be increased at some point in the past to what we currently have and ensured the size
  2. No - because a sub change temporarily adds an element which is later removed on another sub change. We unfortunately can not predict that/count that in beforehand in this ensureSize call. With that said, the second call in addRemove should happen very rarely - probably only with small sizes
  3. It can. When we change an item, lets say A -> B and A is filtered out but B is not, we grow in size by 1 (or the other way around). But same as in the permutation,- the ensureSize should be a noop, since the size had to be increased at some point in the past to what we currently have and ensured the size

Copy link
Copy Markdown
Contributor

@andy-goryachev-oracle andy-goryachev-oracle May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, while I agree that it's likely to be a no-op, my original comment did pointed out a possible optimization (not that it will give us megabytes or free space or 10% less CPU consumption).

So in cases 1) and 2) we could call ensureSize() only when needed. It's up to you whether you want to pursue this further or leave as is.

Copy link
Copy Markdown
Member Author

@Maran23 Maran23 May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought about it as well - but this is very hard to balance.
We reserve quite much space here. If we have 1000 elements, we reserve space for 1501 elements. That's huge. And we are in FilteredList - so the normal usecase is probably with quite a few elements filtered out.

But on the other hand growing in size just a few times is much better performance-wise vs. growing in size many more times. As I found out here already: #758


beginChange();
while (c.next()) {
if (c.wasPermutated()) {
Expand Down Expand Up @@ -236,7 +238,6 @@ private void permutate(Change<? extends E> c) {

private void addRemove(Change<? extends E> c) {
Predicate<? super E> pred = getPredicateImpl();
ensureSize(getSource().size());
final int from = findPosition(c.getFrom());
final int to = findPosition(c.getFrom() + c.getRemovedSize());

Expand Down Expand Up @@ -269,6 +270,7 @@ private void addRemove(Change<? extends E> c) {
// Add the remaining elements
while (it.nextIndex() < c.getTo()) {
if (pred.test(it.next())) {
ensureSize(size + 1);
System.arraycopy(filtered, fpos, filtered, fpos + 1, size - fpos);
filtered[fpos] = it.previousIndex();
nextAdd(fpos, fpos + 1);
Expand All @@ -282,7 +284,6 @@ private void addRemove(Change<? extends E> c) {

private void update(Change<? extends E> c) {
Predicate<? super E> pred = getPredicateImpl();
ensureSize(getSource().size());
int sourceFrom = c.getFrom();
int sourceTo = c.getTo();
int filterFrom = findPosition(sourceFrom);
Expand Down
17 changes: 17 additions & 0 deletions modules/javafx.base/src/test/java/test/javafx/collections/FilteredListTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import com.sun.javafx.collections.ObservableListWrapper;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -352,4 +353,20 @@ public void testSortedThenFilteredListDoesNotThrowIOOBE() {
assertEquals(List.of(0L), filteredList);
assertEquals(List.of(0L), sortedList);
}

@Test
public void testMoveItemDoesNotThrowAIOOBE() {
var list = new ObservableListWrapper<>(new ArrayList<>(List.of(1L)));
FilteredList<Long> filteredList = new FilteredList<>(list);

list.add(2L);
assertEquals(List.of(1L, 2L), filteredList);

ObservableListWrapperShim.beginChange(list);
list.remove(2L);
list.addFirst(2L);
assertDoesNotThrow(() -> ObservableListWrapperShim.endChange(list));

assertEquals(List.of(2L, 1L), filteredList);
}
}