Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Add ArrayDequeue #490

Merged
merged 5 commits into from
Mar 9, 2018
Merged

Add ArrayDequeue #490

merged 5 commits into from
Mar 9, 2018

Conversation

julienrf
Copy link
Contributor

@julienrf julienrf commented Mar 6, 2018

I picked the content of #49, fixed the remaining compilation errors, and rebased on master.

In summary, the PR introduces the ArrayDeque collection and implements Queue on top of it. MutableList and LinkedList are removed.

@julienrf julienrf requested a review from szeiger March 6, 2018 10:22
@@ -124,7 +123,8 @@ private[mutable] class MutableList[A]
this
}

def toQueue = new Queue(first0, last0, len)
@deprecated("Use to(Queue) instead", "2.13.0")
def toQueue: Queue[A] = to(Queue)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Queue used to be built on top of MutableList, but that’s not the case anymore with this PR. Consequently I’ve deprecated this method.

with LinearSeqOps[A, Queue, Queue[A]]
class Queue[A] protected(array: Array[AnyRef], start: Int, end: Int)
extends ArrayDeque[A](array, start, end)
with IndexedSeqOps[A, Queue, Queue[A]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Queue switches from being a LinearSeq to an IndexedSeq. This might break existing usage.

this
}

override def slice(from: Int, until: Int): IterableCC[A] = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit hacky: I’ve changed the return type to be IterableCC[A] so that when this operation is inherited by Queue its return type is refined accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

If Queue isn't supposed to override it, shouldn't ofArray return IterableCC, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ofArray is supposed to be overridden.

@julienrf
Copy link
Contributor Author

julienrf commented Mar 6, 2018

@pathikrit I’d be happy to get your feedback on my changes.

@julienrf julienrf added this to the 0.11.0 milestone Mar 6, 2018
@julienrf
Copy link
Contributor Author

julienrf commented Mar 7, 2018

I’ve tried running the benchmarks but for some reason that I ignore appending and prepending elements ot an ArrayBuffer or an ArrayDeque raise a heap memory error (even for small number of elements). Does that sound familiar to someone? I guess the garbage collector is too under pressure?

this
}

override def slice(from: Int, until: Int): IterableCC[A] = {
Copy link
Member

Choose a reason for hiding this comment

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

If Queue isn't supposed to override it, shouldn't ofArray return IterableCC, too?

* @param destStart
* @param maxItems
*/
def copySliceToArray(srcStart: Int, dest: Array[_], destStart: Int, maxItems: Int): dest.type = {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn’t need.

requireBounds(srcStart)
val startIdx = start_+(srcStart)
val block1 = Math.min(toCopy, array.length - startIdx)
Array.copy(src = array, srcPos = startIdx, dest = dest, destPos = destStart, length = block1)
Copy link
Member

Choose a reason for hiding this comment

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

copySliceToArray is usually called with new array as dest and destStart = 0. This case could be optimized to avoid initializing the start of the array (where the copy of block1 goes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where do we initialize the start of the array?

Copy link
Member

Choose a reason for hiding this comment

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

When you create a new array and pass that to the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry but I don’t see what we could do instead.

Copy link
Member

Choose a reason for hiding this comment

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

Create a different version of copySliceToArray (at least for internal use) that allocates the target array on its own using java.util.Arrays.copyOf. Anyway, this is a minor performance optimization (which we already did in ArrayOps) that can be done later. It shouldn't hold up this PR.

decrementLength()
res
}
def enqueue(elem1: A, elem2: A, elems: A*): this.type = enqueue(elem1).enqueue(elem2).enqueueAll(elems.toStrawman)
Copy link
Member

Choose a reason for hiding this comment

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

This could force a resize to the correct target size before adding any elements if elems.knownSize >= 0

Copy link
Member

@szeiger szeiger left a comment

Choose a reason for hiding this comment

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

The out of memory problem is worrying but since it also happens for ArrayBuffer it should be unrelated to the new implementation here. The other problems I commented on are minor and could be fixed later.

@julienrf julienrf merged commit c0129af into scala:master Mar 9, 2018
@julienrf julienrf deleted the array-dequeue-squashed branch March 9, 2018 19:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants