optimize zip() by pre-sizing target. Fixes #376

[nolequen]

Hope, I found all of them.

[nikhilnanivadekar]

Thanks @nolequen for the PR. From the contract of zip(), we need to pre-size the target collection to the minimum size of the two iterables.
For example: please refer to: immutablePrimitiveArrayList.stg#L718
MutableList<<name><name>Pair> target = Lists.mutable.withInitialCapacity(Math.min(size, otherSize));

[nikhilnanivadekar]

Please pre-size the target collection to the minimum size of the two iterables.

[nolequen]

@nikhilnanivadekar, thanks for the feedback! Can you please take a look at the current version.

[nikhilnanivadekar]

@nolequen can you please rebase on to master that will have @donraab 's changes merged on your branch and squash the commits. We like to keep the git log history incremental otherwise it becomes difficult to trace the changes in git log.

Also regarding pre-sizing it using Iterate.sizeOf() I think we would need to do an instance check since Iterate.sizeOf() is O(n) for Iterable which is not a Collection or RichIterable. See here: L3183
There is 1 place where pre-sizing is done for select I have commented on the relevant line.

[nikhilnanivadekar]

It is ok to not pre-size in this file. Since the max the zipped Bag will be is of size 1. Pre-sizing it to 1 defaults it to 10.

[nolequen]

As far as I see in objectPrimitiveHashMap.stg pre-sizing to 1 gives map capacity=2 while without presizing it gives capacity=16.

[nolequen]

@nikhilnanivadekar what kind of instance check do you mean? can you provide an example?

[nikhilnanivadekar]

Also 1 more request, can you please update the copyright to be 2018 and also add and others.
For Example: https://github.com/eclipse/eclipse-collections/blob/master/site/index.html#L3

[nikhilnanivadekar]

Please revert this file.

[nolequen]

Done.

[nikhilnanivadekar]

We cannot pre-size for select() as we do not know beforehand for how many elements the Predicate will return true. Can you please revert this?

[nolequen]

Sorry for this. Reverted.

[nikhilnanivadekar]

if(that instanceof Collection || that instanceOf RichIterable)
{
    int thatSize = Iterate.sizeOf(that)
    HashBag\<Pair\<V, S>\> target = HashBag.newBag(Math.min(this.size(), thatSize));
    return this.zip(that, target)
}
return this.zip(that, HashBag.\<Pair\<V, S>\>newBag());

[nikhilnanivadekar]

I meant by instance check for only Collection or RichIterable as shown below:

if(that instanceof Collection || that instanceOf RichIterable)
{
    int thatSize = Iterate.sizeOf(that)
    HashBag\<Pair\<V, S>\> target = HashBag.newBag(Math.min(this.size(), thatSize));
    return this.zip(that, target)
}
return this.zip(that, HashBag.\<Pair\<V, S>\>newBag());

[nikhilnanivadekar]

Thanks for your contribution!

[motlin]

Sorry I'm late to this. I just came across this while working on something else. I think this is a deoptimization and we should either fix it or revert it. For context, here's an example implementation today:

@Override
public <S> MutableList<Pair<T, S>> zip(Iterable<S> that)
{
    if (that instanceof Collection || that instanceof RichIterable)
    {
        int thatSize = Iterate.sizeOf(that);
        FastList<Pair<T, S>> target = FastList.newList(Math.min(this.size(), thatSize));
        return this.zip(that, target);
    }
    return this.zip(that, FastList.newList());
}

The problem is the clause that instanceof RichIterable. Not all RichIterables know their size. Specifically:

public interface LazyIterable<T>
        extends RichIterable<T>

If that is an instance of SelectIterable, this will cause two iterations where there used to be one.

@nikhilnanivadekar @nolequen @donraab

[motlin]

Here are two somewhat contrived examples that terminate instantly with 9.1.0 and need to be Ctrl+C killed with the current version.

@Test
public void concatenate()
{
    MutableList<Integer> smallList = Lists.mutable.with(1, 2, 3);

    Interval bigInterval = Interval.zeroTo(Integer.MAX_VALUE);
    LazyIterable<Integer> lazyEmptyInterval = bigInterval.select(x -> false);
    LazyIterable<Integer> concatenate = smallList.asLazy().concatenate(lazyEmptyInterval);

    MutableList<Pair<Integer, Integer>> zipped = smallList.zip(concatenate);
    assertEquals(smallList.zip(smallList), zipped);
}

@Test
public void empty()
{
    MutableList<Integer> emptyList = Lists.mutable.empty();

    Interval bigInterval = Interval.zeroTo(Integer.MAX_VALUE);
    LazyIterable<Integer> lazyEmptyInterval = bigInterval.select(x -> false);

    MutableList<Pair<Integer, Integer>> zipped = emptyList.zip(lazyEmptyInterval);
    assertEquals(Lists.mutable.empty(), zipped);
}

@nikhilnanivadekar @nolequen @donraab