Update for Groovy

[yukoba]

A patch for Groovy.
Read groovy/groovy-core#482 for the detail.

[blackdrag]

sorry, to nag here, but to be able to see what has been changed the patch must be reduced to only logical changes. Please do not apply whitespace changes, and none of the Intellij Idea code "improvements", like changing an if to the ternary operator, don't rename variables and don't reorganize imports. Such things should be seperate commits. Then it is more easy to see things like

- public static <E> ConsPStack<E> from(final Collection<? extends E> list) {
- if(list instanceof ConsPStack)
- return (ConsPStack<E>)list; //(actually we only know it's ConsPStack<? extends E>)
+ public static <E> ConsPStack<E> from(Collection<? extends E> list) {

deleteing these lines is not right, but it could be a github diff view glitch.

[yukoba]

OK, removed the cleanup part.

[yukoba]

Is it really a good idea to call toArray() here?
yukoba@5870e37#src-main-java-org-pcollections-conspstack-java-P42

The computational complexities are these.

	Iterator	My ListIterator	Original ListIterator
Head only	O(1)	O(n)	O(1)
Head to last	O(n)	O(n)	O(n)
Last only		O(n)	O(n)
Last to head		O(n)	O(n^2)

O(n^2) must be avoided.
The maximum acceptable complexity is O(n log n).
Especially, ListIterator is often used for from last to head.

Also, I updated the patch, and changed these.

1. Now PDeque extends PQueue
2. Changed PDeque.plusFirst(Collection) to plusFirstAll(Collection) and plusLast(Collection) to plusLastAll(Collection), because all the other methods which take Collection as an argument have All in the method name.
3. Moved complexity JavaDoc from PDeque to AmortizedPDeque
4. The class of the return value of TreePVector.with() is changed from PVector to TreePVector
5. Added complexity JavaDoc to iterator() and listIterator() of ConsPStack and TreePVector
6. Updated the complexity JavaDoc of AmortizedPDeque.tail() and init() to this

	Average-case	Amortized	Worst-case
Used as Deque	O(1)	O(n)	O(n)
Used as Queue	O(1)	O(1)	O(n)
Used as Stack	O(1)	O(1)	O(1)

[blackdrag]

I agree that list-to-head performance is not good, but I was not talking about computational complexity. I was talking about memory complexity and there the old iterator had a memory requirement of O(1), while the new one has a memory complexity of O(n)

[yukoba]

ConsPStack is already using O(n) space complexity.
It does not change if a O(n) temporary array is used.

[blackdrag]

I did not mean ConsPStack, I did mean the iterator. And while O(2n) is still O(n), it makes still a big difference for finite memory

[yukoba]

Of course I understand it is about ListIterator.
Usually, one ListIterator for one ConsPStack.
Using a temporary array just increases 4 bytes or 8 bytes per each element.
I do not think this is big.

I did mean the iterator

I believe you do not misunderstand,
this is not about Iterator, this is about ListIterator.
List uses both.

[blackdrag]

yeah, we are both speaking about ListIterator;) 4 or 8 bytes is not much, but as you said, it is per element. This means of your ConsPStack consumes half of the available memory, it is possible, that creating the ListIterator will cause an OutOfMemoryException, while using the normal Iterator will work.

But maybe it is ok enough to add a big warning to the javadoc of that method about it not being "in situ" / in-place

[yukoba]

consumes half of the available memory

It is not half!

If the element is using 64 bytes,
and if Object[] uses 8 bytes per element, the increase is 12.5%.
Also ConsPStack is already using at least 8 + 8 + 4 = 20 bytes per element.
In any case, it is smaller than half.

[blackdrag]

you are right

[hrldcpr]

Sadly I think this and groovy/groovy-core#482 are stale now, so even though it looks like there's some cool stuff in here I'm going to close this as it's too big for me to sift through to find the gems.

But if there are any parts of this that could be rescued as their own smaller PRs that would be welcome!