Skip to content
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

Jump to bottom

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

Open
donraab opened this issue May 19, 2024 · 4 comments · May be fixed by #1657
Open

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

donraab opened this issue May 19, 2024 · 4 comments · May be fixed by #1657

Comments

@donraab
Copy link
Contributor

donraab commented May 19, 2024

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
Copy link
Contributor

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

This comment was marked as resolved.

Sign in to view
@fermendivilt
Copy link
Contributor

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:

@fermendivilt fermendivilt linked a pull request Aug 2, 2024 that will close this issue
Open
@motlin
Copy link
Contributor

motlin commented Aug 3, 2024

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;
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@motlin @donraab @fermendivilt