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 33 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
5 changes: 3 additions & 2 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ scalaVersion in ThisBuild := "2.12.2-ebe1180-SNAPSHOT" // from https://github.co
scalaBinaryVersion in ThisBuild := "2.12"

scalacOptions in ThisBuild ++=
Seq("-deprecation", "-unchecked", "-Yno-imports", "-language:higherKinds")
Seq("-deprecation", "-unchecked", "-language:higherKinds")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not remove this option to avoid confusion with by-default imported collections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, an artifact of this being ported from an external repo: pathikrit/scalgos@bd1d794

Will put back all imports


testOptions in ThisBuild += Tests.Argument(TestFrameworks.JUnit, "-q", "-v", "-s", "-a")

Expand All @@ -21,7 +21,8 @@ val collections =
name := "collection-strawman",
libraryDependencies ++= Seq(
"org.scala-lang.modules" %% "scala-java8-compat" % "0.8.0",
"com.novocode" % "junit-interface" % "0.11" % Test
"com.novocode" % "junit-interface" % "0.11" % Test,
"org.scalatest" %% "scalatest" % "3.0.1" % Test
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not sure we can afford such a dependency, especially if our goal is to eventually move to the scala/scala repository.

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 moved the code from another of my repos: pathikrit/scalgos@bd1d794 and thus it dragged in scalatest - I am fine porting tests to junit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Speaking of tests - is it possible to write a property test which takes mutable.ArrayBuffer and mutable.ArrayDeque and basically does a fuzz test i.e. any sequence of random operations on them to prove that one can be a drop-in replacement for the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. I'm not sure that there's much value-add for that above what collections-laws should already do (explicitly test relationships that should always hold, with fuzzing of data but not operations), but if it's not too hard to manage it's probably a good idea before we make the switch.

),
pomExtra :=
<developers>
Expand Down
349 changes: 349 additions & 0 deletions src/main/scala/strawman/collection/mutable/ArrayDeque.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,349 @@
package scala //TODO: Change this strawman --> we just need sizeHintIfCheap from scala
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we actually do this? I don't think we're likely to add any new collections before strawman hits.

Copy link
Contributor Author

@pathikrit pathikrit Aug 5, 2017

Choose a reason for hiding this comment

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

How do I get access to sizeHintIfCheap if I am not in Scala package?

Copy link
Contributor

Choose a reason for hiding this comment

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

You add it to the strawman collections?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have a knownSize member, which returns -1 if the size is not cheap to compute. Would that work for you?

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 knownSize would work. QQ: Why is it returning -1 instead of Option[Int]? Perf?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another question - I understand why knownSize might return -1 for say Streams or Iterators but what about somethlng like Lists? The idea in the previous API was the sizeHintIfCheap would only give you size in O(1) if possible. But, now, knownSize does not apply to List - for lists the size is known but is expensive O(n) to calculate. I think sizeIfCheap was more apt in that case.

Copy link
Contributor

@julienrf julienrf Aug 8, 2017

Choose a reason for hiding this comment

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

For List it returns -1. Maybe the name can be improved. But I guess the scaladoc is clearer:

The number of elements in this collection, if it can be cheaply computed, -1 otherwise. Cheaply usually means: Not requiring a collection traversal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, sizeIfCheap makes more sense here then knownSize since former implies size is not known because of expensive-ness and the latter implies size is not known because of something could be an infinite stream

Copy link
Contributor

Choose a reason for hiding this comment

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

It was meant to convey whether or not you already know the size, not whether it is knowable / finite.

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.collection.{GenSeq, generic, mutable}
import scala.reflect.ClassTag

/** 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.12
* @since 2.12
*
* @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.

set(array, start, end)
Copy link
Contributor

Choose a reason for hiding this comment

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

set is too generic of a name for a method with this particular functionality.


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)
get(idx).asInstanceOf[A]
Copy link
Contributor

Choose a reason for hiding this comment

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

get is too ubiquitous of a method name for this internal functionality. At least use _get or myGet or something with a private-suggesting name.

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 will change the private set and get to _set and _get. But does it matter? We are not Python and there is a private access modifier on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a problem for the people maintaining the code. Many collections have a get method (e.g. maps). Keeping the name the same as a public method in another part of the hierarchy is just asking for confusion if someone is trying to make a quick fix or get a quick idea of how something works.

}

override def update(idx: Int, elem: A) = {
ArrayDeque.checkIndex(idx, this)
set(idx, elem.asInstanceOf[AnyRef])
Copy link
Contributor

Choose a reason for hiding this comment

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

set is also too ubiquitous of a name

}

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]): this.type = {
// Note this is faster than super.++=: because this does not need to convert TraversableOnce to a Traversable
if (elems.isEmpty) return this
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to have a return here. It's not nested that deeply.

elems.sizeHintIfCheap match {
case srcLength if srcLength >= 0 && 2*(srcLength + this.length) >= mask =>
val finalLength = srcLength + this.length
val array2 = ArrayDeque.alloc(finalLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a lot of allocating and shuffling going on here. Can't you just preallocate and stuff everything into the beginning with prependAssumingCapacity? I can't imagine all these allocations could be faster.

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 only allocating if I know I have to resize i.e. final length is > 2x current length.

elems.copyToArray(array2.asInstanceOf[Array[A]])
copySliceToArray(srcStart = 0, dest = array2, destStart = srcLength, maxItems = size)
set(array = array2, start = 0, end = finalLength)
case _ =>
val copy = clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

Ow, you don't need a copy of the whole thing. Just copy the array.

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 I can't copy the array since it might be in a circular order here i.e. first element of array might be the middle element of collection

Copy link
Contributor

Choose a reason for hiding this comment

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

But you know the endpoints. You can handle it manually. I guess this does reduce the chance of implementation error.

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.

If you copy just the array, you'll have to walk element by element here, but that's not a big deal. (Or call some method that does it for you.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this faster? See ++=

Copy link
Contributor

Choose a reason for hiding this comment

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

I just meant that ++= copy can be inlined to access the copied array directly. If you're dealing with arrays of length 5 or something, I bet it makes a big difference.

}
this
}

override def insertAll(idx: Int, elems: scala.collection.Traversable[A]): Unit = {
ArrayDeque.checkIndex(idx, this)
if (elems.isEmpty) return
elems.sizeHintIfCheap 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)
set(array = array2, start = 0, end = finalLength)
} else {
val suffix = drop(idx)
end = start_+(idx)
elems.foreach(appendAssumingCapacity)
suffix.foreach(appendAssumingCapacity)
Copy link
Contributor

Choose a reason for hiding this comment

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

Using foreach on suffix is unnecessarily expensive. Having suffix produced by drop is also unnecessarily expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you suggest do I do here otherwise?

Copy link
Contributor

Choose a reason for hiding this comment

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

You keep track of the indices and do it by hand off of the array.

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 : Done: 1647625

}
case _ => //expensive to compute size
val suffix = drop(idx)
end = start_+(idx)
this ++= elems ++= suffix
}
}

override def remove(idx: Int, count: Int): Unit = {
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.

The logic here is in the wrong order. If you remove nothing, it doesn't matter whether you're out of bounds.

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 will follow what other collections do here. Intuitively, it seems I should see if idx is valid and then worry about count. It seems strange that remove(idx = -1, count = randomInt().signum) can fail only 50% of the time depending on the count parameter ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I don't know, deleting nothing is a no-op so it probably shouldn't throw an exception?

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)
set(array = array2, start = 0, end = size - removals)
} else if (size - idx <= idx + removals) {
var i = idx
while(i + removals < size) {
set(i, get(i + removals))
set(i + removals, null)
i += 1
}
nullify(from = i)
end = end_-(removals)
} else {
var i = idx + removals - 1
while(i - removals >= 0) {
set(i, get(i - removals))
set(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() = new ArrayDeque(array.clone, start, end)
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 clone on an array is as fast as, say, java.util.Arrays.copyOf(array, array.length). For small arrays the extra wrappers can add up (or at least used to).

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 : Isn't clone() intrinsic to JVM? Reverted back here: 35ae179

Copy link
Contributor

Choose a reason for hiding this comment

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

I just tested and it seems like the difference only shows up for primitives. (clone has to figure out the underlying type of the array, whereas copyOf on a primitive does not.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, since it's an array of AnyRef, clone is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, specifically for Array[AnyRef], I still do see a difference but it's so small (~half a nanosecond) it's probably not worrying about.


/**
* 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")
set(array = ArrayDeque.alloc(size), start = 0, end = 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the size is already correct, you shouldn't allocate again.

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 if I don't allocate again, I need to loop through and clear out the array for GC. For non-trivial lengths, this seems faster.

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)

@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?


@inline private[this] def end_-(idx: Int) = (end - idx) & mask

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

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

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

private[this] def set(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
}

private[this] def resize(len: Int) = {
val array2 = copySliceToArray(srcStart = 0, dest = ArrayDeque.alloc(len), destStart = 0, maxItems = size)
set(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

/**
* Allocates an array whose size is next power of 2 > $len
*
* @param len
* @return
*/
private[ArrayDeque] def alloc(len: Int) = {
val i = len max DefaultInitialSize
new Array[AnyRef](((1 << 31) >>> Integer.numberOfLeadingZeros(i)) << 1)
}

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