Skip to content
This repository was archived by the owner on Dec 22, 2021. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
73 commits
Select commit Hold shift + click to select a range
fcaa01a
First commit of ArrayDeque for collections-strawman
pathikrit Mar 24, 2017
834d9d7
switch to scalatest
pathikrit Mar 24, 2017
51e4f4a
upgrade scala version
pathikrit Mar 24, 2017
e6838e6
First pass through code review suggestions
pathikrit Mar 27, 2017
57dbc41
move checkIndex to companion object
pathikrit Mar 27, 2017
faba4cd
rename arrayCopy to copySliceToArray
pathikrit Mar 27, 2017
4f94cdd
break foreach+if into 2 while loops
pathikrit Mar 27, 2017
aa20b6c
change scalaversion to original
pathikrit Mar 27, 2017
fa6c7e0
DRY setting nulls
pathikrit Mar 27, 2017
436a665
make defaultInitialSize final
pathikrit Mar 27, 2017
56ca57b
remove ++=:
pathikrit Mar 27, 2017
65133dc
inline fencePost util
pathikrit Mar 27, 2017
fab70a1
use Any instead of AnyRef
pathikrit Mar 27, 2017
ebbbb2a
use array copy for larger insertAll
pathikrit Mar 27, 2017
a17f38c
faster insertAll
pathikrit Mar 27, 2017
5f137d5
switch back to using AnyRef arrays
pathikrit Mar 27, 2017
e418495
faster reverse
pathikrit Mar 27, 2017
b955281
use sizeHintIfCheap in insertAll
pathikrit Mar 27, 2017
c82c14e
fix typo
pathikrit Mar 28, 2017
aca8957
faster reverse
pathikrit Mar 28, 2017
804bf43
implement sizeHint
pathikrit Mar 28, 2017
595d6c5
downsize in removeFirst/removeLast
pathikrit Mar 28, 2017
7892282
overwrite ++=:
pathikrit Mar 29, 2017
0cde632
Add clear and shrink
pathikrit Mar 29, 2017
f7ed01c
Give user option to resize when using removeFirst/removeLast
pathikrit Mar 29, 2017
c09de3a
privatize internal constructor and state
pathikrit Mar 29, 2017
1ecaf48
faster prepend
pathikrit Mar 29, 2017
0f203ad
sliding works when windowSize > stepSize
pathikrit Jun 27, 2017
27a8e6d
Document alternative sliding
pathikrit Jun 27, 2017
49bb07a
Simplify sliding
pathikrit Jun 27, 2017
c9063c6
Make the assumingCapacity utils return this
pathikrit Jun 27, 2017
64bc294
DRY-up bitmasking
pathikrit Jun 27, 2017
185d61c
:lipstick:
pathikrit Jun 28, 2017
68c9933
First pass through code review suggestions
pathikrit Aug 7, 2017
3640fa6
Merge remote-tracking branch 'upstream/master'
pathikrit Aug 7, 2017
076d6f5
Use junit for tests
pathikrit Aug 7, 2017
42ff0e3
Switch to strawman.collection.mutable package
pathikrit Aug 7, 2017
bcab165
mimic behaviour of ArrayBuffer when count<0 and idx is out of bounds
pathikrit Aug 7, 2017
b0b6d78
do not resize if current size is fine
pathikrit Aug 7, 2017
38147a5
PoC implementations of mutable.{Stack, Queue} using mutable.ArrayDeque
pathikrit Aug 8, 2017
87ea88b
change names for private methods
pathikrit Aug 8, 2017
33c7e67
optimize prependAll and insertAll
pathikrit Aug 8, 2017
f0e8ea8
Remove dependency on Predef.scala
pathikrit Aug 9, 2017
891e519
DRY up mutable.{Stack, Queue}
pathikrit Aug 9, 2017
35ae179
Go back to Array.clone()
pathikrit Aug 9, 2017
36f27c4
Remove internal mask variable
pathikrit Aug 9, 2017
d46dd19
Optimized `++=:`
pathikrit Aug 9, 2017
6c48477
simpler bound check
pathikrit Aug 9, 2017
96d996d
Consistent naming of remove APIs
pathikrit Aug 9, 2017
1dfc995
Faster Reverse APIs
pathikrit Aug 9, 2017
2c93ade
Optimized insertAll for inserts at beginning/end
pathikrit Aug 9, 2017
2bb2c75
Remove redundant check for emptiness
pathikrit Aug 9, 2017
57b2fb6
Optimized removals
pathikrit Aug 10, 2017
1647625
Optimize insertAll
pathikrit Aug 10, 2017
360e4bf
Internal DSL for modular arithmetic
pathikrit Aug 10, 2017
959410d
Optimize removeAll()
pathikrit Aug 10, 2017
b4cdc9b
Better resize strategy for removals
pathikrit Aug 10, 2017
de9c2ec
Do not repeatedly resize smaller collections
pathikrit Aug 10, 2017
0c47056
Remove unused utils
pathikrit Aug 10, 2017
efd6a47
Do not import scala._
pathikrit Aug 10, 2017
ed1ace7
Add benchmarks
pathikrit Aug 10, 2017
7addd8b
Improve benchmarks
pathikrit Aug 11, 2017
4b54e93
Move ArrayDequeBenchmark to benchmarks module
pathikrit Aug 11, 2017
755261e
Consistent length/size usages
pathikrit Aug 12, 2017
cb35330
Merge remote-tracking branch 'upstream/master'
pathikrit Dec 9, 2017
968aa5f
Move to correct directory
pathikrit Dec 9, 2017
f832409
Add self to developers (to trigger a CI build post CLA sign check)
pathikrit Dec 9, 2017
7b2b9e1
First cut at porting to Scala 2.13
pathikrit Dec 9, 2017
8c4a20c
Move Stack/Queue to use strawman.collection.Seq
pathikrit Dec 9, 2017
92f2252
Merge branch 'master' into master
pathikrit Jan 31, 2018
d3d7155
Rename removeHeadWhile to removeWhile
pathikrit Jan 31, 2018
b8c32a6
Move peek to queue/stack APIs
pathikrit Jan 31, 2018
0519263
remove reverseIterator
pathikrit Jan 31, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
369 changes: 369 additions & 0 deletions src/main/scala/strawman/collection/mutable/ArrayDeque.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,369 @@
package strawman
package collection.mutable

Choose a reason for hiding this comment

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

Is this implementation supposed to fit in the current library or in the rewritten one? I thought it was supposed to go to this repo to fit in the rewritten library, but then it should not use the existing standard collection library. Instead:

  • This file is in strawman/collection but in package scala.collection.mutable?
  • This file uses the standard collection library...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need .sizeHintIfCheap which is package visible to scala.collection.mutable only.

Choose a reason for hiding this comment

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

But if you put a class in package scala.collection.mutable, it belongs in directory scala/collection/mutable.

But more fundamentally, IIUC collections in this repo don't use scala.collection at all, since the point of this repo is to completely redesign that package for 2.13. @julienrf am I missing something?
I don't know if this should be ported to the 2.13 API, but it seems that should be clarified before focusing on low-level details.

I don't know how much effort it'd be to port this to collection.strawman, but it's probably non-trivial.

Because of the binary compatibility constraints, this code cannot be added to 2.12.x (and this would have been the wrong repo anyway) as long as it changes the API (even adding methods/classes is forbidden, because of forward compatibility—clients built with 2.12.2 must run against 2.12.0). It can be shipped as an external library for 2.12.x users, though it's maybe better then to put it in another package and give up on the optimization (adding to an existing package from a new JAR is forbidden by OSGi).

Because the APIs you're using will change a lot for 2.13, this code can't be merged as-is in 2.13 either.

Not my decision of course—it's just my understanding of the existing architectural rules.

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 repo, as far as I understand, is a sandbox/playground type repo to prototype ideas before it lands in Scala. I just need access to .sizeHintIfCheap - which would presumably exist in future Traversable too right?

Making it use collection.strawman is also quite trivial. Just revert this commit: b955281

It does not really use any other Scala collection besides Arrays underneath...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, indeed we should put new collections into strawman.collection. That will make it easier to have both the old and new collections at the same time, so that we can compare them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DarkDimius : So, this rewrite will happen for 2.13 or a further future version of Scala? I would like to get this into Scala sooner than later (if we have to wait for a full rewrite of collections). Should I send this pull request against Scala then?

Copy link
Contributor

Choose a reason for hiding this comment

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

The plan is to target 2.13.

As @DarkDimius said, for now if you need something that’s related to the current collections you should just copy-paste it in the strawman.

Choose a reason for hiding this comment

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

@pathikrit, yes. This is repo is targeting 2.13.

In order to release sooner you should target https://github.com/scala/scala, but I don't think your contribution will be included in 2.12 lifecycle due to binary compatibility considerations. If your collection is included in, lets say, 2.12.3, it's clearly incompatible with 2.12.0, as it didn't include this collection.

Based on this, I think that the earliest you collection can be included is next binary incompatible release, which is 2.13. And this means that you should target this repo and base your contributions on this strawman.

Another alternative would be for you to release it as an artifact which is a separate artifact on maven.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine with 2.13. Happy to make whatever changes necessary for that. I just don't want to wait for 2.14 or Dotty or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DarkDimius : Another alternative would be to include this as a private collection in scala 2.12.3 and change the underlying implementation of mutable.{Queue, Stack, Buffer} to simply proxy to this - since this is strictly better than all of those (I have some hacky benchmarks but not a full suite to post yet).


import scala._
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this import? We added the Yno-predef flag to not have our scope polluted with scala.List or other default collection-esque imports from scala.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: efd6a47

import scala.collection.{GenSeq, generic, mutable}
import scala.reflect.ClassTag
import scala.Predef._

import java.lang.Integer

/** An implementation of a double-ended queue that internally uses a resizable circular buffer
* Append, prepend, removeFirst, removeLast and random-access (indexed-lookup and indexed-replacement)
* take amortized constant time. In general, removals and insertions at i-th index are O(min(i, n-i))
* and thus insertions and removals from end/beginning are fast.
*
* @author Pathikrit Bhowmick
* @version 2.13
* @since 2.13
*
* @tparam A the type of this ArrayDeque's elements.
*
* @define Coll `mutable.ArrayDeque`
* @define coll array deque
* @define thatinfo the class of the returned collection. In the standard library configuration,
* `That` is always `ArrayDeque[B]` because an implicit of type `CanBuildFrom[ArrayDeque, B, ArrayDeque[B]]`
* is defined in object `ArrayDeque`.
* @define bfinfo an implicit value of class `CanBuildFrom` which determines the
* result class `That` from the current representation type `Repr`
* and the new element type `B`. This is usually the `canBuildFrom` value
* defined in object `ArrayDeque`.
* @define orderDependent
* @define orderDependentFold
* @define mayNotTerminateInf
* @define willNotTerminateInf
*/
@SerialVersionUID(1L)
class ArrayDeque[A] private[ArrayDeque](
private[ArrayDeque] var array: Array[AnyRef],
private[ArrayDeque] var start: Int,
private[ArrayDeque] var end: Int
) extends mutable.AbstractBuffer[A]
with mutable.Buffer[A]
with generic.GenericTraversableTemplate[A, ArrayDeque]
with mutable.BufferLike[A, ArrayDeque[A]]
with mutable.IndexedSeq[A]
with mutable.IndexedSeqOptimized[A, ArrayDeque[A]]
with mutable.Builder[A, ArrayDeque[A]]
with Serializable {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have a comment somewhere in here describing the invariants that ArrayDeque is supposed to maintain. Off the top of my head, array has length of a power of two, mask is equal to array.length-1 (actually, do we need to store this in a field? A single decrement is super-fast), and start and end are...actually...I'm not sure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These invariants are there on line 316 inside set

Copy link
Contributor

Choose a reason for hiding this comment

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

That's an awkward place for them, isn't it? But, okay, as long as they're somewhere.


private[this] var mask = 0 // modulus using bitmask since array.length is always power of 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to have this as a constructor parameter? You have to give it the right size of array anyway, so you already know what the mask is going to be. And then you don't have to set it to zero and then try to patch it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because sometimes I resize internally and patch up mask. But I am okay with removing this entirely but & mask reads much nicer than & (array.length - 1). Also isn't the former faster?

Copy link
Contributor

Choose a reason for hiding this comment

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

Math is really fast, and the array length will generally be accessed anyway. Might be worth a quick benchmark on something you think ought to be affected.

resetInternal(array, start, end)

private[this] def resetInternal(array: Array[AnyRef], start: Int, end: Int) = {
this.array = array
this.mask = array.length - 1
assert((array.length & mask) == 0, s"Array.length must be power of 2")
assert(0 <= start && start <= mask && 0 <= end && end <= mask)
this.start = start
this.end = end
}

def this(initialSize: Int = ArrayDeque.DefaultInitialSize) = this(ArrayDeque.alloc(initialSize), 0, 0)

override def apply(idx: Int) = {

Choose a reason for hiding this comment

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

It would be good to have an explicit type annotation on these methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually don't put the types if I put override since its redundant (you cant make a mistake here) but not a strong opinion

Choose a reason for hiding this comment

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

True, but since this is public API, I think the redundancy is worth it for the sake of clarity.

ArrayDeque.checkIndex(idx, this)
getInternal(idx).asInstanceOf[A]
}

override def update(idx: Int, elem: A) = {
ArrayDeque.checkIndex(idx, this)
setInternal(idx, elem.asInstanceOf[AnyRef])
}

override def +=(elem: A) = {
sizeHint(size)
appendAssumingCapacity(elem)
}

override def +=:(elem: A) = {
sizeHint(size)
prependAssumingCapacity(elem)
}

@inline private[ArrayDeque] def appendAssumingCapacity(elem: A): this.type = {
array(end) = elem.asInstanceOf[AnyRef]
end = end_-(-1)
this
}

@inline private[ArrayDeque] def prependAssumingCapacity(elem: A): this.type = {
start = start_+(-1)
array(start) = elem.asInstanceOf[AnyRef]
this
}

override def ++=:(elems: TraversableOnce[A]) = {
// Note this is faster than super.++=: because this does not need to convert TraversableOnce to a Traversable
if (elems.nonEmpty) {
ArrayDeque.knownSize(elems) match {
case srcLength if srcLength >= 0 && 2*(srcLength + this.length) >= mask =>
val finalLength = srcLength + this.length
val array2 = ArrayDeque.alloc(finalLength)
elems.copyToArray(array2.asInstanceOf[Array[A]])
copySliceToArray(srcStart = 0, dest = array2, destStart = srcLength, maxItems = size)
resetInternal(array = array2, start = 0, end = finalLength)
case _ =>
val copy = clone()
end = start
this ++= elems ++= copy
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check whether the extra copy of the entire data structure (instead of just the valid part of the array) has negligible cost? Another option is to prepend elems and then swap elements to fix up the order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have another idea that I have not fleshed out fully yet. Basically, if I keep track of "direction of iteration" in another boolean/int internal private variable, I can toggle it to provide O(1) in-place reverse API which can be used to reduce code duplication between append and prepend. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt the overhead will be worth it. You'll have an extra branch on basically every operation. For core library stuff like this, reducing code duplication is good when it comes without significant drawbacks, but otherwise I think other concerns (predictable behavior, useful interface, good performance) come first.

Copy link
Contributor Author

@pathikrit pathikrit Aug 7, 2017

Choose a reason for hiding this comment

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

I don't think it will be a branch overhead. Basically I will have private[this] var dir: Int = 1

This is how reverseInPlace() would look like:

def reverseInPlace(): this.type = {
  val temp = start
  start = end
  end = temp
  dir = -dir
  this
}

Every other operation remains the same except I replace (start + idx) & mask with start + dir*idx) & mask etc. This would not affect locality either for arrays.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW the same idea is used ​in the Spandex PR #52 to get constant time reverse (albiet expressed via flipping the start and stop fields to avoid adding a specific reversed field). Only a few operations needed to be updated to take the direction into account, most of the directional logic could be hidden in internal (inlined) methods. It did not affect the benchmarks negatively in any significant way (it did affect the reverse benchmark quite positively though 🤓).

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think through all the code, so if you want to try it and benchmark it, great! But I do think it's important to do benchmarks. It will also, I think, make it harder to find correctness problems because of the added difficulty of thinking through the logic. I think we have or can create enough tests, but despite the reduction in code duplication I think complicating e.g. all the array copy stuff is going to be a net loss to clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I tried to implement this and it is 2x slower when array is reversed - but for a surprisingly different reason (nothing to do with indexing is slow). This is because copySliceToArray is the most critical section of this data structure and it uses memcpy via System.arrayCopy underneath - when dir is -1 (i.e. array is reversed) there is no way to memcopy in reverse order fast (unless you loop which is ~2x slower than System.arrayCopy).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay! Good to know--thanks for checking. I guess this is good enough. We'll worry about the wrapper overhead some other time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ichoran - here's an alternative more optimized version - 33c7e67

I am sure more can be done using memcopy...

}
}
this
}

override def insertAll(idx: Int, elems: scala.collection.Traversable[A]): Unit = {
ArrayDeque.checkIndex(idx, this)
if (elems.nonEmpty) {
ArrayDeque.knownSize(elems) match {
case srcLength if srcLength >= 0 =>
val finalLength = srcLength + this.length
// Either we resize right away or move prefix right or suffix left
if (2*finalLength >= mask) {
val array2 = ArrayDeque.alloc(finalLength)
copySliceToArray(srcStart = 0, dest = array2, destStart = 0, maxItems = idx)
elems.copyToArray(array2.asInstanceOf[Array[A]], idx)
copySliceToArray(srcStart = idx, dest = array2, destStart = idx + srcLength, maxItems = size)
resetInternal(array = array2, start = 0, end = finalLength)
} else {
val suffix = drop(idx)
end = start_+(idx)
elems.foreach(appendAssumingCapacity)
suffix.foreach(appendAssumingCapacity)
}
case _ => //expensive to compute size
val suffix = drop(idx)
end = start_+(idx)
this ++= elems ++= suffix
}
}
}

override def remove(idx: Int, count: Int): Unit = {
if (count < 0) throw new IllegalArgumentException(s"removing negative number of elements: $count")
ArrayDeque.checkIndex(idx, this)
if (count == 0) return
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking the index should come after one knows there's something to remove (as in current collections). Also, the logic should probably be

if (count <= 0) {
  if (count < 0) throw ...
  else return
}

just in case the JIT compiler doesn't rewrite it that way for efficiency on the success/do-something case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started a broader discussion for this: https://stackoverflow.com/questions/45559608/api-design-mixing-in-pre-condition-checks-for-index-out-of-bounds

Meanwhile it currently behaves exactly like mutable.ArrayBuffer ..

val removals = (size - idx) min count
// If we are removing more than half the elements, its cheaper to start over
// Else, choose the shorter: either move the prefix (0 until (idx + removals) right OR the suffix (idx to size) left
if (2*removals >= size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

2*removals cannot overflow, but for weird reasons: your longest possible ArrayDeque is only 2^30 - 1 elements. You should document this limitation in the Scaladoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Java's ArrayDeque has same limitation - will document. If you are trying to allocate a 8GB collection, good luck to you! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not 8GB, just 1.1GB is a problem.

val array2 = ArrayDeque.alloc(size - removals)
copySliceToArray(srcStart = 0, dest = array2, destStart = 0, maxItems = idx)
copySliceToArray(srcStart = idx + removals - 1, dest = array2, destStart = idx, maxItems = size)
resetInternal(array = array2, start = 0, end = size - removals)
} else if (size - idx <= idx + removals) {
var i = idx
while(i + removals < size) {
setInternal(i, getInternal(i + removals))
setInternal(i + removals, null)
i += 1
}
nullify(from = i)
end = end_-(removals)
} else {
var i = idx + removals - 1
while(i - removals >= 0) {
setInternal(i, getInternal(i - removals))
setInternal(i - removals, null)
i -= 1
}
nullify(until = i + 1)
start = start_+(removals)
}
}

override def remove(idx: Int) = {
val elem = this(idx)
remove(idx, 1)
elem
}

/**
*
* @param resizeInternalRepr If this is set, resize the internal representation to reclaim space once in a while
* @return
*/
def removeFirst(resizeInternalRepr: Boolean = false): Option[A] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Secret boolean arguments aren't the best style. Let's just decide whether we're resizing or not, and write a single argument. (Or two differently-named arguments where it's clear which is which.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's give user the maximum flexibility (i.e. both versions that resize or not). I don't have strong opinions about using 2 methods vs. one with boolean arg.

if (isEmpty) {
None
} else {
val elem = array(start)
array(start) = null
start = start_+(1)
if (resizeInternalRepr && 2*size < mask) resize(size)
Some(elem.asInstanceOf[A])
}
}

/**
*
* @param resizeInternalRepr If this is set, resize the internal representation to reclaim space once in a while
* @return
*/
def removeLast(resizeInternalRepr: Boolean = false): Option[A] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing with resize boolean.

if (isEmpty) {
None
} else {
end = end_-(1)
val elem = array(end)
array(end) = null
if (resizeInternalRepr && 2*size < mask) resize(size)
Some(elem.asInstanceOf[A])
}
}

override def reverse = foldLeft(new ArrayDeque[A](initialSize = size))(_.prependAssumingCapacity(_))

override def sizeHint(hint: Int) = if (hint >= mask) resize(hint + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this handles Int.MaxValue correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm certainly not going to because that code is GPLed and this code is not. If you've been using that as an extensive reference, I'm afraid we'll have to reimplement this all from scratch :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this does not use any part of the Java code - it is completely different implementation.


override def length = end_-(start)

override def isEmpty = start == end

override def nonEmpty = start != end

override def clone() = {
// TODO Scala's array.clone should call java.util.Arrays.copyOf(array, array.length)
new ArrayDeque(java.util.Arrays.copyOf(array, array.length), start, end)
}
/**
* Note: This does not actually resize the internal representation.
* See clearAndShrink if you want to also resize internally
*/
override def clear() = {
nullify()
start = 0
end = 0
}

Choose a reason for hiding this comment

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

This is now inconsistent with the auto-shrinking, given my earlier example of clearing, then adding 101, then removing 1, etc.

I feel like there are two options for consistency sake:

Behavior 1: no auto-shrink on removals, no shrinking on clear
Behavior 2: auto-shrink on removals, shrink on clear

Either this is represented in types ArrayDeque + ArrayDeque with ManualShrink or a subclass; or there is extra internal state to track if there has been a size request. I'm not personally a fan of the extra internal state but others might not be a fan of an extra type. To minimize differences, both can have clearNoShrink and clearAndShrink but one implements clear with one and the other type implements its clear with the other. Additionally, the two types would need to differ on whether there was any auto-shrinking during removals.

What are other opinions on this? I see a few more use cases now where auto-shrink is useful -- I was thinking along the lines of when I would use a Buffer, but Queue and Deque use cases that I can imagine have more situations where auto-shrink would be useful. I tend to think of a Buffer as an intermediate data structure during transformation of other data structures. Whereas a Queue and Deque are often longer lived.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cakes don't play nicely with map and friends.

Internal state is the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not a fan of sizeHint request being made as implicit behaviour changing. I think the current state of the API is fine. By default, both clear and removeFirst/removeLast APIs do not shrink. The remove(x, count) API does shrink but only if we are removing more than half the elements - which is a reasonable choice.

If the user wants to manage memory herself, she can either call .trimToSize explicitly at the end OR call .clearAndShrink instead of .clear and call .removeFirst(autoShrink=true) (which will shrink if size is less than half of allocation).


/**
* clears this buffer and shrinks to @param size
*
* @param size
* @return
*/
def clearAndShrink(size: Int = ArrayDeque.DefaultInitialSize): this.type = {
require(size >= 0, s"Non-negative size required")
resetInternal(array = ArrayDeque.alloc(size), start = 0, end = 0)
this
}

override def slice(from: Int, until: Int) = {
val left = if (from <= 0) 0 else if (from >= size) size else from
val right = if (until <= 0) 0 else if (until >= size) size else until
Copy link
Contributor

Choose a reason for hiding this comment

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

You never actually use right except to calculate len. May as well just calculate len directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep you are right - just made the code easier to read :)

val len = right - left
if (len <= 0) {
ArrayDeque.empty[A]
} else if (len >= size) {
clone()
} else {
val array2 = copySliceToArray(srcStart = left, dest = ArrayDeque.alloc(len), destStart = 0, maxItems = len)
new ArrayDeque(array2, 0, len)
}
}

override def sliding(window: Int, step: Int) = {
require(window > 0 && step > 0, s"window=$window and step=$step, but both must be positive")
val lag = (window - step) max 0
Iterator.range(start = 0, end = length - lag, step = step).map(i => slice(i, i + window))
}

override def grouped(n: Int) = sliding(n, n)

override def copyToArray[B >: A](dest: Array[B], destStart: Int, len: Int) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Having this extra method that just overrides the second argument to be 0 while having a different name seems awkward. ("What is copyToArray? What is arrayCopy? Why aren't these the same, and which one is more general?")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well copyToArray is something we inherit (see it has override on it). The arrayCopy is much more general than the one I am forced to overwrite since it also takes in a srcStart unlike copyToArray. Will document this.

copySliceToArray(srcStart = 0, dest = dest, destStart = destStart, maxItems = len)

override def companion = ArrayDeque

override def result() = this

override def stringPrefix = "ArrayDeque"

override def toArray[B >: A: ClassTag] =
copySliceToArray(srcStart = 0, dest = new Array[B](size), destStart = 0, maxItems = size)

/**
* This is a more general version of copyToArray - this also accepts a srcStart unlike copyToArray
* This copies maxItems elements from this collections srcStart to dest's destStart
* If we reach the end of either collections before we could copy maxItems, we simply stop copying
*
* @param dest
* @param srcStart
* @param destStart
* @param maxItems
*/
def copySliceToArray(srcStart: Int, dest: Array[_], destStart: Int, maxItems: Int): dest.type = {
ArrayDeque.checkIndex(srcStart, this)
ArrayDeque.checkIndex(destStart, dest)
val toCopy = maxItems min (size - srcStart) min (dest.length - destStart)
if (toCopy > 0) {
val startIdx = start_+(srcStart)
val block1 = toCopy min (array.length - startIdx)
Array.copy(src = array, srcPos = startIdx, dest = dest, destPos = destStart, length = block1)
Copy link
Contributor

Choose a reason for hiding this comment

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

My recollection is that java.util.Arrays.copyOfRange is more efficient for this use case, but I forget why and don't have time to check right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? Both this and copyOfRange call system.arrayCopy. See line 3294 here: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/Arrays.java#3253

if (block1 < toCopy) {
Array.copy(src = array, srcPos = 0, dest = dest, destPos = block1, length = toCopy - block1)
}
}
dest
}

/**
* Trims the capacity of this ArrayDeque's instance to be the current size
*/
def trimToSize(): Unit = resize(size - 1)

/**
* Add idx to start modulo mask
*/
@inline private[this] def start_+(idx: Int) = (start + idx) & mask
Copy link
Contributor

Choose a reason for hiding this comment

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

Cute name but I'm not sure I entirely approve. It takes a few second-takes to realize what's going on (then it's pretty cool).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to alternative suggestion :) Maybe startModuloPlus(idx: Int)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just startPlus?


/**
* Subtract idx from end modulo mask
*/
@inline private[this] def end_-(idx: Int) = (end - idx) & mask

@inline private[this] def getInternal(idx: Int) = array(start_+(idx))

@inline private[this] def setInternal(idx: Int, elem: AnyRef) = array(start_+(idx)) = elem

private[this] def nullify(from: Int = 0, until: Int = size) = {
var i = from
while(i < until) {
setInternal(i, null)
i += 1
}
}

private[this] def resize(len: Int) = {
val array2 = copySliceToArray(srcStart = 0, dest = ArrayDeque.alloc(len), destStart = 0, maxItems = size)
resetInternal(array = array2, start = 0, end = size)
}
}

object ArrayDeque extends generic.SeqFactory[ArrayDeque] {
implicit def canBuildFrom[A]: generic.CanBuildFrom[Coll, A, ArrayDeque[A]] =
ReusableCBF.asInstanceOf[GenericCanBuildFrom[A]]

override def newBuilder[A]: mutable.Builder[A, ArrayDeque[A]] = new ArrayDeque[A]()

final val DefaultInitialSize = 8

private[ArrayDeque] def knownSize[A](coll: TraversableOnce[A]) = {
/*coll.sizeHintIfCheap*/ -1 //TODO: Remove this temporary util when we switch to strawman .sizeHintIfCheap is now .knownSize
}

/**
* Allocates an array whose size is next power of 2 > $len
* Largest possible len is 1<<30 - 1
*
* @param len
* @return
*/
private[ArrayDeque] def alloc(len: Int) = {
val size = ((1 << 31) >>> Integer.numberOfLeadingZeros(len max DefaultInitialSize)) << 1
new Array[AnyRef](size.ensuring(_ >= 0, s"ArrayDeque too big - cannot allocate ArrayDeque of length $len"))
}

@inline private[ArrayDeque] def checkIndex(idx: Int, seq: GenSeq[_]) =
if (!seq.isDefinedAt(idx)) throw new IndexOutOfBoundsException(idx.toString)
}
Loading