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 6 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
12 changes: 7 additions & 5 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ organization in ThisBuild := "ch.epfl.scala"

version in ThisBuild := "0.2.0-SNAPSHOT"

resolvers in ThisBuild += "scala-pr" at "https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots"
scalaVersion in ThisBuild := "2.12.2-ebe1180-SNAPSHOT" // from https://github.com/scala/scala/pull/5742
//resolvers in ThisBuild += "scala-pr" at "https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots"
//scalaVersion in ThisBuild := "2.12.2-ebe1180-SNAPSHOT" // from https://github.com/scala/scala/pull/5742
scalaVersion in ThisBuild := "2.12.1"

Choose a reason for hiding this comment

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

Please drop this version change. scala/scala#5742 (comment) actually says that bugfix is needed here:

Rebased on top of 2.12.x head in order to build a version that also includes #5708, so I can use it in collection-strawman.

but dropping this version change from the PR is a good idea anyway.

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: aa20b6c

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")
//testOptions in ThisBuild += Tests.Argument(TestFrameworks.JUnit, "-q", "-v", "-s", "-a")

fork in Test in ThisBuild := true

Expand All @@ -21,7 +22,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
268 changes: 268 additions & 0 deletions src/main/scala/strawman/collection/mutable/ArrayDeque.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,268 @@
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.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(var array: Array[AnyRef], var start: Int, var end: Int)
Copy link
Member

Choose a reason for hiding this comment

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

final?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggesting make this a final class?

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) = {
ensureCapacity()
array(end) = elem.asInstanceOf[AnyRef]
end = (end + 1) & mask
this
}

override def +=:(elem: A) = {
ensureCapacity()
start = (start - 1) & mask
array(start) = elem.asInstanceOf[AnyRef]
this
}

override def ++=:(xs: TraversableOnce[A]) = //TODO: Improve this - foldRight is expensive
xs.foldRight(this)((x, coll) => x +=: coll).asInstanceOf[this.type]
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks correct but is hardly an efficient implementation. Can you mark this in some way for later improvement? (foldRight is an expensive operation for a lot of 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.

Hmm, what would be an efficient one? I will add a TODO here for now...

Choose a reason for hiding this comment

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

Couldn't this use insertAll?

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 ++=: (prependAll) not ++= (insertAll)

Choose a reason for hiding this comment

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

prependAll(xs) is the same as insertAll(0, xs), no? I assumed that making sure the space is available beforehand and then copying the elements would be more efficient than adding them one by one like we do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh, you are right! In fact, that's what it is in the super. Will remove.


override def insertAll(idx: Int, elems: scala.collection.Traversable[A]) = {
ArrayDeque.checkIndex(idx, this)
val src = elems.toBuffer
/*val finalLength = src.length + this.length
// Either we resize right away or move prefix right or suffix left
if (2*finalLength >= array.length) {
val array2 = ArrayDeque.alloc(finalLength)
arrayCopy(dest = array2, srcStart = 0, destStart = 0, maxItems = idx)
Array.copy(src = src, srcPos = 0, dest = array2, destPos = idx, length = src.length)
arrayCopy(dest = array2, srcStart = idx, destStart = idx + src.length, maxItems = size)
set(array = array2, start = 0, end = finalLength)
} else {*/
// TODO: choose to move prefix right or suffix left
val suffix = drop(idx)
end = (start + idx) & mask
this ++= src ++= 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, either move the prefix right or the suffix left - whichever is shorter
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) {
//TODO: Instead of if in the loop, break into 2 loops
(idx until size) foreach {i =>
val elem = if (i + removals < size) get(i + removals) else null
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 conditional inside the loop seems like a bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea - will break into 2 loops

set(i, elem)
}
end = (end - removals) & mask
} else {
//TODO: Instead of if in the loop, break into 2 loops
(0 until (idx + removals)).reverse foreach {i =>

Choose a reason for hiding this comment

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

All these operations on ranges could get expensive. Might be more straightforward to use a while loop here anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmethvin / @Ichoran : changed 2 while loops:
4f94cdd

I think there is still scope for improvement by using Java's array copy here (atleast for setting nulls) - the previous section is tricky since it is moving a slice of array to itself...

val elem = if (i - removals < 0) null else get(i - removals)
set(i, elem)
}
start = (start + removals) & mask
}
}

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

def removeFirst(): Option[A] = {
if (isEmpty) {
None
} else {
val elem = array(start)
array(start) = null
start = (start + 1) & mask
Some(elem.asInstanceOf[A])
}
}

def removeLast(): Option[A] = {
if (isEmpty) {
None
} else {
end = (end - 1) & mask
val elem = array(end)
array(end) = null
Some(elem.asInstanceOf[A])
}
}

override def length = (end - start) & mask

Choose a reason for hiding this comment

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

To call this method, this whole file calls size — to find its implementation when reviewing ensureCapacity I had to find override def size = length here. Calling length directly in the rest of the code would ease review.

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 thought it was pretty well known that size and length are interchangeable for all collections?

Choose a reason for hiding this comment

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

Yes, but I saw size, and when I found no definition I just deduced "it's from a superclass somehow" (I thought in Buffer).

But this is certainly a small thing, and disappears with an IDE.


override def isEmpty = 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.


override def clear() = if (nonEmpty) set(array = ArrayDeque.alloc(ArrayDeque.defaultInitialSize), start = 0, end = 0)

override def slice(from: Int, until: Int) = {
val left = fencePost(from)
val right = fencePost(until)
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 >= 1 && step >= 1, s"size=$size and step=$step, but both must be positive")
(indices by step).iterator.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) & mask
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 CircularBuffer's instance to be the current size
*/
def trimToSize(): Unit = resize(size - 1)

@inline private def fencePost(i: Int) = if (i <= 0) 0 else if (i >= size) size else i

@inline private def get(idx: Int) = array((start + idx) & mask)

@inline private def set(idx: Int, elem: AnyRef) = array((start + idx) & mask) = elem

@inline private 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")
this.start = start
this.end = end
}

private def ensureCapacity() = {
// We resize when we are 1 element short intentionally (and not when array is actually full)
// This is because when array is full, start = end and it is hard to recognize then if array is actually full or empty
if (size == array.length - 1) resize(array.length)
}

private 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]()

val defaultInitialSize = 8

Choose a reason for hiding this comment

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

Since this is a constant it should be DefaultInitialSize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything's a val (constant) in Scala?

Copy link

Choose a reason for hiding this comment

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

why not DEFAULT_INITIAL_SIZE

Copy link

Choose a reason for hiding this comment

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

why not final val DEFAULT_INITIAL_SIZE = 8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hepin1989 I think you are confusing Java with Scala. final here in Scala means it is not overridable and not that it is a const

Copy link
Contributor

Choose a reason for hiding this comment

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

@pathikrit - Actually, SLS section 4.1 says,

A constant value definition is of the form

final val x = e

where e is a constant expression (§6.24). The final modifier must be present and
no type annotation may be given. References to the constant value x are themselves
treated as constant expressions; in the generated code they are replaced by the def-
inition’s right-hand side e.

Copy link
Contributor

Choose a reason for hiding this comment

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

And FWIW this does make a significant performance difference in tight code (e.g. constants used for simple random number generators).

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 : I agree but we are talking about the naming of the variable with a capital letter or not here :)

Choose a reason for hiding this comment

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

I'm referring to constants as the term is used in the Scala style guide:

Constant names should be in upper camel case. Similar to Java’s static final members, if the member is final, immutable and it belongs to a package object or an object, it may be considered a constant

I don't have a really strong opinion on this, but I assumed that we would follow the official Scala style guide here. That seems to distinguish constants from simple values.

Also, I agree with @Ichoran the final modifier should be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmethvin : Done: 436a665


/**
* 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)
}
46 changes: 46 additions & 0 deletions src/test/scala/strawman/collection/test/ArrayDequeSpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package strawman
package collection.test

import strawman.collection.mutable.ArrayDeque

import org.scalatest._

import scala.collection.mutable

class ArrayDequeSpec extends FlatSpec {
"circular-buffer" should "match the library" in {
val buffer = ArrayDeque.empty[Int]
val buffer2 = mutable.ArrayBuffer.empty[Int]

def apply[U](f: mutable.Buffer[Int] => U) = {
//println(s"Before: [buffer1=${buffer}; buffer2=${buffer2}]")
assert(f(buffer) === f(buffer2))
assert(buffer === buffer2)
}

apply(_.append(1, 2, 3, 4, 5))
apply(_.prepend(6, 7, 8))
apply(_.trimStart(2))
apply(_.trimEnd(3))
apply(_.insertAll(0, Seq(9, 10, 11)))
apply(_.insertAll(1, Seq(12, 13)))
apply(_.remove(2))
apply(_.prependAll(Seq(14, 15, 16, 17)))
apply(_.remove(1, 5))
apply(_.prependAll(Seq.tabulate(100)(identity)))
buffer.trimToSize()
apply(_.appendAll(Seq.tabulate(100)(identity)))

(-100 to 100) foreach {i =>
assert(buffer.splitAt(i) === buffer2.splitAt(i))
}

for {
i <- -100 to 100
j <- -100 to 100
} {
assert(buffer.slice(i, j) === buffer2.slice(i, j))
if (i >= 1 && j >= 1 && j >= i) assert(buffer.sliding(i, j).toList === buffer2.sliding(i, j).toList)
}
}
}