Perf Issue: toImmutable[List/Set/Bag] methods should be overridden on LazyIterable

[donraab]

The default implementations of toImmutable[List/Set/Bag] are suboptimal for LazyIterable. Calls to isEmpty or size should be avoided for LazyIterable as they will force iteration to happen. The following tests will fail currently and should be added with appropriate fixes, which should probably be just overriding with default methods in LazyIterable.

@Test
public void perfIssueInLazyIterableToImmutableList()
{
    MutableBag<Integer> visited = Bags.mutable.empty();
    LazyIterable<Integer> integers = Interval.oneTo(10);
    LazyIterable<Integer> select =
            integers.select(i -> i > 3)
                    .tap(visited::add)
                    .select(i -> i < 7)
                    .tap(visited::add);
    ImmutableList<Integer> immutableList = select.toImmutableList();

    Assertions.assertEquals(List.of(4, 5, 6), immutableList);

    // Expected: [4, 4, 5, 5, 6, 6, 7, 8, 9, 10]
    // Actual: [4, 4, 4, 4, 5, 5, 6, 6, 7, 8, 9, 10, 4, 4, 5, 5, 6, 6, 7, 8, 9, 10]
    Assertions.assertEquals(Bags.mutable.of(4, 4, 5, 5, 6, 6, 7, 8, 9, 10), visited);
}

@Test
public void perfIssueInLazyIterableToImmutableBag()
{
    MutableBag<Integer> visited = Bags.mutable.empty();
    LazyIterable<Integer> integers = Interval.oneTo(10).toBag().asLazy();
    LazyIterable<Integer> select =
            integers.select(i -> i > 3)
                    .tap(visited::add)
                    .select(i -> i < 7)
                    .tap(visited::add);
    Bag<Integer> immutableBag = select.toImmutableBag();

    Assertions.assertEquals(Bags.mutable.of(4, 5, 6), immutableBag);

    // Expected: [4, 4, 5, 5, 6, 6, 7, 8, 9, 10]
    // Actual: [4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 6, 6, 6, 6, 7, 7, 8, 8, 9, 9, 10, 10]
    Assertions.assertEquals(Bags.mutable.of(4, 4, 5, 5, 6, 6, 7, 8, 9, 10), visited);
}

@Test
public void perfIssueInLazyIterableToImmutableSet()
{
    MutableBag<Integer> visited = Bags.mutable.empty();
    LazyIterable<Integer> integers = Interval.oneTo(10).toBag().asLazy();
    LazyIterable<Integer> select =
            integers.select(i -> i > 3)
                    .tap(visited::add)
                    .select(i -> i < 7)
                    .tap(visited::add);
    ImmutableSet<Integer> immutableBag = select.toImmutableSet();

    Assertions.assertEquals(Set.of(4, 5, 6), immutableBag);

    // Expected: [4, 4, 5, 5, 6, 6, 7, 8, 9, 10]
    // Actual: [4, 4, 4, 4, 4, 4, 5, 5, 5, 5, 6, 6, 6, 6, 7, 7, 8, 8, 9, 9, 10, 10]
    Assertions.assertEquals(Bags.mutable.of(4, 4, 5, 5, 6, 6, 7, 8, 9, 10), visited);
}

[fermendivilt]

Hello, I would like to work on this issue! Since there is no one assigned, I will start working on it and report back when solved,,, or if I get stuck :)

[fermendivilt]

Reporting: I am stuck :)

Thought I had solved it out until now, but suddenly it came back: I could run the tests in another project that imported eclipse-collections, but when I try to run them from the fork, I get the compilation error about an unread package.

I am using IntelliJ and manage the JDK and Maven with it. Here is an image of the error when trying to compile eclipse-collections-api:

[fermendivilt]

To avoid confusion with my hidden comment: Tomorrow I will create a pull request with a solution for the issue. Good night to anyone reading this c:

[motlin]

I was going to say that the implementation should use toArray()

    /**
     * Converts the LazyIterable to the default ImmutableList implementation.
     */
    @Override
    default ImmutableList<T> toImmutableList()
    {
        T[] array = (T[]) this.toArray();
        return Lists.immutable.with(array);
    }

    /**
     * Converts the LazyIterable to the default ImmutableSet implementation.
     */
    @Override
    default ImmutableSet<T> toImmutableSet()
    {
        T[] array = (T[]) this.toArray();
        return Sets.immutable.with(array);
    }

    /**
     * Converts the LazyIterable to the default ImmutableBag implementation.
     */
    @Override
    default ImmutableBag<T> toImmutableBag()
    {
        T[] array = (T[]) this.toArray();
        return Bags.immutable.with(array);
    }

However I see that toArray() has the same problem and needs to be fixed too.

    @Override
    public Object[] toArray()
    {
        Object[] result = new Object[this.size()];
        this.forEachWithIndex((each, index) -> result[index] = each);
        return result;
    }

[motlin]

This was largely addressed in #1744 but I'd like to keep this issue open as a reminder to also optimize the toArray() method in a similar way.