SI-10167: Implement mutable.ArrayDeque #49

pathikrit · 2017-03-24T21:33:41Z

Prior discussion:
https://contributors.scala-lang.org/t/using-circular-buffers-as-mutable-arraybuffer/454

SI-10167: https://issues.scala-lang.org/browse/SI-10167

ArrayDeque's (implemented using a growable circular buffers using modular arithmetic for indexing) are strictly better data structures than vanilla ArrayBuffers.

This improves on java.util.ArrayDeque by providing an O(1) get(idx) and also providing faster insertAt(idx) and deleteAt(idx) operations.

This implementation adds O(1) append, prepend, removeHead, removeTail, get, update and clear APIs.

We also provide proof of concept replacement implementations of mutable data structures like stack, queues and buffers with ArrayDeque here.

Preliminary benchmarks prove that this data structure can suitably replace mutable.ArrayBuffer

===============[Insert lots of items]=================
  ArrayDeque:   956.76 ms
 ArrayBuffer:  2473.36 ms
===============[Drop some items from an head index]=================
  ArrayDeque:     1.25 ms
 ArrayBuffer:     7.65 ms
===============[Drop some items from a tail index]=================
  ArrayDeque:     0.28 ms
 ArrayBuffer:     2.54 ms
===============[Append lots of items one by one]=================
  ArrayDeque:  2222.13 ms
 ArrayBuffer:  3576.63 ms
===============[Prepend few items one by one]=================
  ArrayDeque:     1.33 ms
 ArrayBuffer:  8699.13 ms
===============[Prepend lots of items at once]=================
  ArrayDeque:   462.76 ms
 ArrayBuffer:  2124.02 ms
===============[Random indexing]=================
  ArrayDeque:    84.02 ms
 ArrayBuffer:    81.62 ms
===============[Insert items near head]=================
  ArrayDeque:  1429.52 ms
 ArrayBuffer:  2980.46 ms
===============[Reversal]=================
  ArrayDeque:   378.69 ms
 ArrayBuffer:   491.46 ms
===============[Insert items near tail]=================
  ArrayDeque:  2504.20 ms
 ArrayBuffer:  8588.98 ms
===============[Sliding]=================
  ArrayDeque:   157.25 ms
 ArrayBuffer:  1591.47 ms
===============[toArray]=================
  ArrayDeque:   181.07 ms
 ArrayBuffer:   194.55 ms
===============[Clear lots of items]=================
  ArrayDeque:    48.34 ms
 ArrayBuffer:    28.62 ms

Ichoran · 2017-03-25T15:14:32Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+
+  @inline private def checkIndex(idx: Int, seq: GenSeq[_] = this) = if(!seq.isDefinedAt(idx)) throw new IndexOutOfBoundsException(idx.toString)
+
+  @inline private def box(i: Int) = if (i <= 0) 0 else if (i >= size) size else i


This is not a good choice of name for what this does. clip is fine (or something longer), but "box" usually refers to wrapping of a value in another object.

Seems like the intention here is to find a location at which to slice, so maybe call this slicePoint.

Also this could be written as i max 0 min size. I don't have a strong opinion either way but if you're using max and min already you might as well use them consistently throughout the code.

For inlined code, it's better to avoid value class invocations of numeric methods--the JVM has to work hard to get rid of all of the extra stuff left in the bytecode after you type i max 0 min size. The if-statement is preferable.

@Ichoran Fine with me. I saw the min and max methods used elsewhere here so I'm mainly advocating for some consistency. We can use if statements or math.min/math.max directly.

For inlined code, it's better to avoid value class invocations of numeric methods--the JVM has to work hard to get rid of all of the extra stuff left in the bytecode after you type i max 0 min size

OT: That's probably true but it saddens me greatly Scalac lacks an optimizer and requires us to write assembly.

@Ichoran: "boxing" a number into a range is standard parlance but in context of jvm, I agree it might be misleading. Changed it to fencePostfen

@gmethvin : this function is a critical section of the code.. so preferring if here. The places where I did use min/max are in non-critical paths (e.g. during amortized growing) or places that are O(n) anyway..

Ichoran · 2017-03-25T15:17:41Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+    var i = len max defaultInitialSize
+    //See: http://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2
+    i |= i >> 1; i |= i >> 2; i |= i >> 4; i |= i >> 8; i |= i >> 16
+    Array.ofDim[AnyRef](i + 1)


Why not just new Array[AnyRef](i+1) and cut out all the indirection?

Ichoran · 2017-03-25T15:22:13Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+  private def ensureCapacity() = if (size == array.length - 1) resize(array.length)
+
+  private def resize(len: Int) = {
+    val array2 = ArrayDeque.alloc(len)


Is it worth special-casing small additions to avoid the sequential bit-shifting and all these tests when you're just adding a single element at a time? At large sizes it doesn't matter but at small sizes the five shift/or/equals might be a non-negligible part of the total runtime.

I think this is reasonably since the default initial size is 16 cells. If you look at Java's ArrayDeque - it uses the same idea..

Ichoran · 2017-03-25T15:27:16Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+      set(array = array2, start = 0, end = size - removals)
+    } else if (size - idx <= idx + removals) {
+      (idx until size) foreach {i =>
+        val elem = if (i + removals < size) get(i + removals) else null


Having this conditional inside the loop seems like a bad idea.

Good idea - will break into 2 loops

Ichoran · 2017-03-25T15:30:01Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+  }
+
+  override def ++=:(xs: TraversableOnce[A]) =
+    xs.foldRight(this)((x, coll) => x +=: coll).asInstanceOf[this.type]


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)

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

Ichoran · 2017-03-25T15:33:29Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+    */
+  def trimToSize(): Unit = resize(size - 1)
+
+  @inline private def checkIndex(idx: Int, seq: GenSeq[_] = this) = if(!seq.isDefinedAt(idx)) throw new IndexOutOfBoundsException(idx.toString)


This seems like too much generality for the fastest cases, and is a bit confusing besides. (Since the version without this seems like it ought to somehow be aware of this. But it isn't.)

okay, will remove the default param...

Ichoran · 2017-03-25T15:35:44Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+
+  override def grouped(n: Int) = sliding(n, n)
+
+  override def copyToArray[B >: A](dest: Array[B], destStart: Int, len: Int) =


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?")

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.

gmethvin · 2017-03-25T21:57:13Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

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


Since this is a constant it should be DefaultInitialSize.

Everything's a val (constant) in Scala?

why not DEFAULT_INITIAL_SIZE

why not final val DEFAULT_INITIAL_SIZE = 8

@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

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

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

@Ichoran : I agree but we are talking about the naming of the variable with a capital letter or not here :)

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.

@gmethvin : Done: 436a665

gmethvin · 2017-03-25T22:05:15Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+    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) {


gmethvin · 2017-03-25T22:09:06Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+  }
+
+  override def ++=:(xs: TraversableOnce[A]) =
+    xs.foldRight(this)((x, coll) => x +=: coll).asInstanceOf[this.type]


Couldn't this use insertAll?

This is ++=: (prependAll) not ++= (insertAll)

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.

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

gmethvin · 2017-03-25T22:11:27Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+
+  def this(initialSize: Int = ArrayDeque.defaultInitialSize) = this(ArrayDeque.alloc(initialSize), 0, 0)
+
+  override def apply(idx: Int) = {


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

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

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

gmethvin · 2017-03-25T22:24:06Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+      }
+      end = (end - removals) & mask
+    } else {
+      (0 until (idx + removals)).reverse foreach {i =>


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

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

gmethvin · 2017-03-25T22:32:25Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+
+  @inline private def checkIndex(idx: Int, seq: GenSeq[_] = this) = if(!seq.isDefinedAt(idx)) throw new IndexOutOfBoundsException(idx.toString)
+
+  @inline private def box(i: Int) = if (i <= 0) 0 else if (i >= size) size else i


Seems like the intention here is to find a location at which to slice, so maybe call this slicePoint.

gmethvin · 2017-03-25T22:40:19Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+
+  @inline private def checkIndex(idx: Int, seq: GenSeq[_] = this) = if(!seq.isDefinedAt(idx)) throw new IndexOutOfBoundsException(idx.toString)
+
+  @inline private def box(i: Int) = if (i <= 0) 0 else if (i >= size) size else i


Also this could be written as i max 0 min size. I don't have a strong opinion either way but if you're using max and min already you might as well use them consistently throughout the code.

Blaisorblade · 2017-03-26T20:25:53Z

Regarding the Travis failure https://travis-ci.org/scala/collection-strawman/builds/214811920, it's a StackOverflow on -Xss6M O_O

[info] Compiling 3 Scala sources to /home/travis/build/scala/collection-strawman/target/scala-2.12/test-classes...
java.lang.StackOverflowError

edmundnoble · 2017-03-27T03:07:15Z

I have a question: in the case of repeatedly removing an element from the end and adding an element to the beginning, is this still more performant than ArrayBuffer? Can you name any edge cases for performance?

Blaisorblade · 2017-03-27T11:42:32Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+    this.end = end
+  }
+
+  private def ensureCapacity() = if (size == array.length - 1) resize(array.length)


This will never reduce the array size. When size is less than 1/4 of array.length, you can halve the array size and keep the amortized O(1) costs.

That's subtle though: when size is only less than 1/2 of array.length, you can't halve the array size. Otherwise, if you take an array half full, you can add and remove elements to cause a reallocation every, say, two elements, and get amortized O(n) cost.

ensureCapacity is used internally to grow the array to next powers of 2... The user must call trimToSize to explicitly resize to a smaller size (same as in Java's ArrayDequeue). We dont want to resize all the time we do a add/remove.

I agree we shouldn't reallocate all the time. But the trick I suggest comes from Cormen's algorithm book, which proves it "doesn't reallocate too often". Technically, it proves the amortized complexity (sort of the average complexity over operations, though not really) is still O(1), though the constant factors can be worse.

OTOH, getting this right is a bit subtle as I mentioned. And if the Java code doesn't use this trick, I guess there might be tradeoffs, including the constant factors.
On the other hand, calling trimToSize too often decreases performance, not calling it wastes space, and calling it smartly... is often a complicated way of doing the same thing. I'll believe you can do better when I see it.
So I'd suggest to still auto-decrease space by default, and maybe have a flag for more manual management for more advanced scenarios, if there's a demonstrated use-case.

The issue is that one might call clear() on a large buffer, then add a bunch of elements one at a time after that in order to avoid extra allocation. In that use case, having it shrink at being below 1/4 full is a bad idea. Another situation is if the user pre-sizes it to say, 2MB then starts adding things one at a time. We would not want it to shrink on the first element added in that case either. Nor would we want it to shrink if we added 100 things, removed 1, added 100, removed 1, etc (e.g. only checking on removal). It will take a long time before it reaches more than 1/4 full, and it is too hard to know inside of ensureCapacity() what the user's intention is.

And since every append / prepend needs to ensureCapacity(), it needs to be as simple and fast as possible for the job.

A separate maybeShrink() applied selectively might work, but I'm not sure what the right heuristics for that would be -- retain the initial size? track a recent clear() call? That all seems messy to me.

I think if the api did not have a clear() and was purely append/prepend/removeTail/removeHead then auto-shrink at the right time is a better fit. I would go as far as to say that a structure that auto-shrinks is a different data structure entirely, with a slightly different use case. I'd rather have users ask for that by type, something like ShrinkingArrayDeque. However, I can't think of a use case where trimToSize is not good enough. Once I am done mutating the buffer, if I intend to hold on to it, I can call that.

Blaisorblade · 2017-03-27T11:46:07Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+    }
+  }
+
+  override def length = (end - start) & mask


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.

I thought it was pretty well known that size and length are interchangeable for all collections?

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.

Blaisorblade · 2017-03-27T12:05:30Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+  private[ArrayDeque] def alloc(len: Int) = {
+    var i = len max defaultInitialSize
+    //See: http://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2
+    i |= i >> 1; i |= i >> 2; i |= i >> 4; i |= i >> 8; i |= i >> 16


This code omits the initial v--; from the reference, yet after a while I confirmed it's correct. Took me a while to understand the code and confirm that this finds the first power of 2 > $len, while the original code finds the first power >= $len. It would be better to elaborate in comments, or this code will look suspicious to other reviewers.

The typical way to implement "round to first power of 2 > len" is:

val nextPowerOf2gtLen = ((1 << 31) >>> Integer.numberOfLeadingZeros(len)) << 1

given that numberOfLeadingZeros is one CPU instruction on most processors.

The graphics.standford.edu tricks are written mostly for C, which does not have a standard way to "invoke" the clz/lzcount CPU instruction.

@Blaisorblade - the comment clearly says 2 > $len on line 260! But good point @sjrd - will change. @sjrd - do you have a reference for "modern bithacks" like this?

No, I don't really have a reference. It's mostly from experience implementing a lot of bit hackery for Scala.js. ;)

Blaisorblade · 2017-03-27T12:30:41Z

in the case of repeatedly removing an element from the end and adding an element to the beginning, is this still more performant than ArrayBuffer?

Oh interesting.
Without checking details, asymptotically, on a correct implementation, each of the removal+addition you describe should take O(n) in an ArrayBuffer, but O(1) in a ArrayDeque because no existing elements are copied.

That only happens as long as you don't have too many reallocations in that sequence. That's exceedingly tricky to verify, but after staring for a while at ensureCapacity, I've concluded that:

it appears to do the right thing under most scenarios I've considered
the code is written very cleverly, with insufficient comments, and it could be made easier to review without performance impact (see my comments).

Blaisorblade · 2017-03-27T12:49:49Z

PS @edmundnoble: haven't tested stuff, so I'm happy to be corrected.

pathikrit · 2017-03-27T16:28:26Z

@Blaisorblade: Do you know why the scalac is throwing a SO here!? Also, yes the ensureCapacity is a bit hard to understand because it relies on 2 things: alloc returning > len (and not >= len) and we resizing when we are 1 element short and not when the cells are actually all full... I will document it in comments...

Blaisorblade · 2017-03-27T16:43:07Z

I only highlighted there's an SO, I'm afraid I can't investigate it or even write a proper bug report.

Ichoran · 2017-03-27T17:01:01Z

src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+    * @param destStart
+    * @param maxItems
+    */
+  def arrayCopy(dest: Array[_], srcStart: Int, destStart: Int, maxItems: Int): Unit = {


Can we call this copyPartToArray or something that better illustrates the difference from copyToArray? Also, to match up with System.arraycopy, the parameters should probably be in the order srcStart, dest, destStart, maxItems.

Blaisorblade · 2017-03-27T17:15:05Z

build.sbt

-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"


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.

done: aa20b6c

pathikrit · 2017-12-09T19:30:40Z

@julienrf : Can you take a look at 7b2b9e1 ? It is my first attempt at following the Scala 2.13 collection style.

Can you tell me if I am in the right direction?

Got most of it to compile except 3 things:

I relied on isTraversableAgain a lot in Scala 2.12 version. What do I do in Scala 2.13??
I relied on .toIndexedSeq in some places to optimize in certain cases. What do I do in Scala 2.13?
I relied on copyToArray a lot. Where is that in Scala 2.13?

If you have time, can you checkout this branch and see an easy way to fix the compile error?
Thank you!

julienrf · 2017-12-09T19:36:47Z

Can you tell me if I am in the right direction?

I had a quick look and that looks good. I’ll take more time to review tomorrow.

I relied on isTraversableAgain a lot in Scala 2.12 version. What do I do in Scala 2.13??

You can do a runtime test:

myIterableOnce match {
  case it: Iterable[A] => …
  case _ => …
}

I relied on .toIndexedSeq in some places to optimize in certain cases. What do I do in Scala 2.13?

We have it:

collection-strawman/collections/src/main/scala/strawman/collection/Iterable.scala

Line 312 in 3c725f2

def toIndexedSeq: immutable.IndexedSeq[A] = immutable.IndexedSeq.from(this)

Otherwise, the generic version is to(IndexedSeq)

szeiger · 2018-01-17T15:29:49Z

@julienrf Do you have an overview what's still missing here? It would be nice to get this merged so I can drop the old mutable.Queue and also revert the ugly changes I made in the compiler codebase due to the lack of a mutable.Stack.

julienrf · 2018-01-17T15:42:08Z

No but I can review the PR for tomorrow.

julienrf

Hey @pathikrit, thanks for your work, we still need to do a few changes before merging it, though. Can you also rebase on top of master (you will have to change add and subtract to addOne and subtractOne) and squash all the commits into a single one?

julienrf · 2018-01-18T09:19:49Z

benchmarks/time/src/main/scala/strawman/collection/mutable/ArrayDequeBenchmark.scala

+  benchmark("Sliding", _.sliding(size = 1000, step = 1000).size)
+  benchmark("Random indexing", b => range10m.foreach(i => if (b.isDefinedAt(i)) b(i)))
+  benchmark("toArray", _.toArray)
+  benchmark("Clear lots of items", _.clear())


I agree that your code is more readable than JMH things but we should use JMH… Could you migrate your benchmark to JMH? You can just copy-paste another benchmark (e.g. the one for ArrayBuffer) and change a few things to make it use ArrayDeque.

We can keep both for now..

julienrf · 2018-01-18T09:51:08Z

collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala

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


In general I prefer omitting the override keyword when I only implement a member, to avoid accidentally overriding something.

I prefer the other way - if I don't put override here, next time some removed the def from the super class, I would be left with a useless method but putting override here would be a compile error if someone removes this from super class

julienrf · 2018-01-18T09:54:27Z

collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+      // The following code resizes the current collection atmost once and traverses elems atmost twice
+      elems.knownSize match {
+        // Size is too expensive to compute AND we can traverse it only once - can't do much but retry with an IndexedSeq
+        case srcLength if srcLength < 0 && !elems.isTraversableAgain => elems.toIndexedSeq ++=: this


Instead of isTraversableAgain you should pattern match on elems:

elems match { case iterable: Iterable[A] => // elems are travarsable again case _ => // elems are traversable only once }

julienrf · 2018-01-18T10:15:02Z

collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+    *  @param f   the predicate used for choosing elements
+    *  @return
+    */
+  def removeHeadWhile(f: A => Boolean): strawman.collection.Seq[A] = {


Since we have take and takeRight, drop and dropRight, maybe we should follow the same convention here and have removeWhile and removeRightWhile?

julienrf · 2018-01-18T10:16:46Z

collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+    *
+    * @return
+    */
+  def peek: Option[A] = headOption


You should make it final and @inline and indicate in the documentation that it is equivalent to headOption. BTW, is it really important to be able to write peek instead of headOption? I would stay with just headOption since this is what we already have.

julienrf · 2018-01-18T10:37:47Z

collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala

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


Why don’t you indicate the type of the elements in the destination array?

def copySliceToArray[B >: A](srcStart: Int, dest: Array[B], destStart: Int, maxItems: Int): Array[B]

?

I get this then:

[error] /Users/pbhowmick/Projects/collection-strawman/collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala:456: type mismatch; [error] found : Array[AnyRef] [error] required: Array[Any] [error] Note: AnyRef <: Any, but class Array is invariant in type T. [error] You may wish to investigate a wildcard type such as `_ <: Any`. (SLS 3.2.10) [error] val array2 = copySliceToArray(srcStart = 0, dest = ArrayDeque.alloc(len), destStart = 0, maxItems = n) [error] ^

Yeah, I saw that too. BTW, have you seen my branch where I fixed all the compilation issues?

julienrf · 2018-01-18T10:42:37Z

collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+  override def apply(idx: Int) = parent(idx)
+
+  override def className = "ArrayDequeView"
+}


It seems to be exactly the same as the default IndexedView instance for IndexedSeq (except for the className).

julienrf · 2018-01-18T10:48:38Z

collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+  def newBuilder[A](): Builder[A, ArrayDeque[A]] =
+    new GrowableBuilder[A, ArrayDeque[A]](empty) {
+      override def sizeHint(size: Int): Unit = {
+        //TODO: elems.ensureSize(size)


Time to uncomment this line :)

julienrf · 2018-01-18T10:52:13Z

collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+object ArrayDeque extends StrictOptimizedSeqFactory[ArrayDeque] {
+   /** Avoid reallocation of buffer if length is known. */
+  def from[B](coll: collection.IterableOnce[B]): ArrayDeque[B] =
+    empty[B].addAll(coll) //TODO: Can be optimized based on coll.size?


Yes, you can surely optimize when knownSize is non negative (as we do in ArrayBuffer).

julienrf · 2018-01-18T10:53:24Z

collections/src/test/scala/strawman/collection/mutable/ArrayDequeTest.scala

@@ -0,0 +1,56 @@
+package strawman
+package collection.test


It would be better to put these tests in the test/junit sub-project.

julienrf · 2018-01-23T15:10:42Z

collections/src/main/scala/strawman/collection/mutable/ArrayDeque.scala

+    while(nonEmpty) {
+      elems += removeHeadAssumingNonEmpty()
+    }
+    elems.result()


This doesn’t seem to be the fastest way to perform this operation. You could copy all the elements in bulk to an ImmutableArray.

julienrf · 2018-01-24T09:42:27Z

Hey @pathikrit, I’ve rebased your work on master, fixed compilation errors and implemented a few missing things. However the tests don’t pass. For instance, prepend doesn’t seem to expand the size of the array. Would you have time to fix those issues? I’ve pushed my code here FYI.

edit: Actually the error was in ArrayBuffer (which is used as a comparison point by ArrayDeque tests).

pathikrit · 2018-01-24T15:00:47Z

@julienrf : Thank you so much! Sorry I got busy and did not get time - I will try to work on it this weekend!

julienrf · 2018-01-24T15:17:53Z

@pathikrit No worries. In the end that looks good! Remaining issues:

the JMH benchmarks take forever to run for += and +=: (which are the two most important operations that we want to benchmark…), I have no idea why (it’s the case for both ArrayBuffer and ArrayDeque)…

This gives me unusable charts like that one:

squash the commits. You don’t have to put everything into a single commit but it would be better to introduce ArrayDeque in a single commit (you can keep having separate commits for fixes to the existing code).

julienrf · 2018-01-24T15:42:43Z

If I run your benchmarks then I get the following:

===============[Insert lots of items]=================
  ArrayDeque:   203.35 ms
 ArrayBuffer:   517.53 ms
===============[Drop some items from an head index]=================
  ArrayDeque:     0.56 ms
 ArrayBuffer:     3.29 ms
===============[Drop some items from a tail index]=================
  ArrayDeque:     0.27 ms
 ArrayBuffer:     0.19 ms
===============[Append lots of items one by one]=================
  ArrayDeque:   508.65 ms
 ArrayBuffer:   505.86 ms
===============[Prepend few items one by one]=================
  ArrayDeque:     1.82 ms
 ArrayBuffer:  6496.78 ms
===============[Insert items near head]=================
  ArrayDeque:   401.04 ms
 ArrayBuffer:   600.07 ms
===============[Reversal]=================
  ArrayDeque:    44.48 ms
 ArrayBuffer:   212.68 ms
===============[Insert items near tail]=================
  ArrayDeque:  9354.62 ms
 ArrayBuffer:   114.26 ms
===============[Sliding]=================
  ArrayDeque:  3353.75 ms
 ArrayBuffer: 11824.66 ms
===============[Random indexing]=================
  ArrayDeque:  3139.11 ms
 ArrayBuffer:   179.65 ms
===============[toArray]=================
  ArrayDeque:    83.60 ms
 ArrayBuffer:  5221.30 ms
===============[Clear lots of items]=================
  ArrayDeque:    40.75 ms
 ArrayBuffer:     0.02 ms

The result for “random indexing” looks weird but I’ve run the benchmark several times and always got results consistent with that one.
Also, it’s unfortunate that clear() is O(n) in ArrayDeque (versus O(1) in ArrayBuffer). Besides that, ArrayDeque looks really great!

pathikrit · 2018-01-24T16:43:13Z

Hmm, random indexing should be fast but this might be an artifact of us micro-benchmarking instead of a real suite. Also, the O(n) clear in ArrayBuffer is a bug right - it would cause memory the JVM to not GC and hold on to a possible large and expensive array that you just cleared.

julienrf · 2018-01-31T10:04:25Z

@pathikrit Any chance you can have a look at it soon?

pathikrit · 2018-01-31T21:36:10Z

Oh just saw your branch - hah, I was progressing towards the same. Looks good - I myself was reaching similar code. Thanks for looking into it.

pathikrit · 2018-01-31T21:38:24Z

@julienrf : What's the next step on this? Can we merge your branch into this? Shall we just open a new clean state PR?

julienrf · 2018-02-01T08:36:34Z

You can just copy-paste my changes to your branch.

julienrf · 2018-03-12T08:40:03Z

Superseded by #490.

pathikrit added 3 commits March 24, 2017 16:09

First commit of ArrayDeque for collections-strawman

fcaa01a

switch to scalatest

834d9d7

upgrade scala version

51e4f4a

Ichoran reviewed Mar 25, 2017

View reviewed changes

pathikrit changed the title ~~Strawman implementation of mutable.ArrayDeque~~ SI-10167: Strawman implementation of mutable.ArrayDeque Mar 25, 2017

gmethvin reviewed Mar 25, 2017

View reviewed changes

Blaisorblade reviewed Mar 27, 2017

View reviewed changes

First pass through code review suggestions

e6838e6

Ichoran reviewed Mar 27, 2017

View reviewed changes

pathikrit added 2 commits March 27, 2017 13:06

move checkIndex to companion object

57dbc41

rename arrayCopy to copySliceToArray

faba4cd

Blaisorblade reviewed Mar 27, 2017

View reviewed changes

pathikrit added 4 commits March 27, 2017 13:26

break foreach+if into 2 while loops

4f94cdd

change scalaversion to original

aa20b6c

DRY setting nulls

fa6c7e0

make defaultInitialSize final

436a665

Move Stack/Queue to use strawman.collection.Seq

8c4a20c

julienrf added the blocker label Jan 3, 2018

julienrf mentioned this pull request Jan 10, 2018

A port of mutable.Queue from the old collections #336

Merged

julienrf suggested changes Jan 18, 2018

View reviewed changes

julienrf reviewed Jan 23, 2018

View reviewed changes

julienrf added this to the 0.10.0 milestone Jan 31, 2018

julienrf mentioned this pull request Jan 31, 2018

ArrayBuffer#clear still references “removed” elements #385

Closed

pathikrit added 4 commits January 31, 2018 14:31

Merge branch 'master' into master

92f2252

Rename removeHeadWhile to removeWhile

d3d7155

Move peek to queue/stack APIs

b8c32a6

remove reverseIterator

0519263

julienrf modified the milestones: 0.10.0, 0.11.0 Mar 1, 2018

julienrf added the in progress label Mar 6, 2018

julienrf mentioned this pull request Mar 6, 2018

Add ArrayDequeue #490

Merged

julienrf closed this Mar 12, 2018


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

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


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

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


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

		val defaultInitialSize = 8


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

		override def apply(idx: Int) = {

SI-10167: Implement mutable.ArrayDeque #49

SI-10167: Implement mutable.ArrayDeque #49

Uh oh!

Conversation

pathikrit commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pathikrit commented Mar 24, 2017 •

edited

Loading