Minor Vector cleanup #5545

paplorinc · 2016-11-18T16:11:27Z

My original intention didn't work out, but since I created these simple style unification commits, I though I'll share them. :)

Ichoran · 2016-11-18T19:41:56Z

src/library/scala/collection/immutable/Vector.scala

@@ -88,7 +83,7 @@ override def companion: GenericCompanion[Vector] = Vector

  override def lengthCompare(len: Int): Int = length - len

-  private[collection] final def initIterator[B >: A](s: VectorIterator[B]) {
+  private[collection] def initIterator[B >: A](s: VectorIterator[B]) {


If we can get away with finality, we should leave it in. In general, immutable data structures do not lend themselves well to subclassing.

the whole class is final, does it make sense to keep final methods also?

Ah. No. Sorry, I missed that. I had somehow assumed, without any evidence, that it was in VectorPointer.

Ichoran · 2016-11-18T19:42:12Z

src/library/scala/collection/immutable/Vector.scala

-  // can still be improved
-  override /*SeqLike*/
-  def reverseIterator: Iterator[A] = new AbstractIterator[A] {
+  override /*SeqLike*/ def reverseIterator: Iterator[A] = new AbstractIterator[A] {


This is harder to read. It's better the original way.

in other places it was formatted like this. should I change the rest also, or just revert this?

If you don't mind changing the others, may as well change them. The override /* SeqLike */ thing is arguably the least important part of the signature, so having it push other stuff out of view on the right, potentially, is a bad idea. Two lines solve the problem nicely.

Ichoran · 2016-11-18T19:43:18Z

src/library/scala/collection/immutable/Vector.scala

-        val shiftBlocks = freeSpace >>> 5*(depth-1) // number of top-level blocks
+        val freeSpace = ((1 << (5 * depth)) - endIndex) // free space at the right given the current tree-structure depth
+        val shift = (freeSpace & ~((1 << (5 * (depth - 1))) - 1)) // number of elements by which we'll shift right (only move at top level)
+        val shiftBlocks = (freeSpace >>> (5 * (depth - 1))) // number of top-level blocks


Spaces are good. Gratuitious parens around the entire expression are not good--just visual clutter that doesn't aid anything. Internal parens that make one not rely upon knowing operator precedence are good.

understood, will remove external ones in other places also :)

Ichoran · 2016-11-18T19:43:48Z

src/library/scala/collection/immutable/Vector.scala

-        val shiftBlocks = startIndex >>> 5*(depth-1)
-
-        //println("----- appendBack " + value + " at " + endIndex + " reached block end")
+        val shift = startIndex & ~((1 << 5 * (depth - 1))-1)


Missed spaces around the last -

Ichoran · 2016-11-18T19:44:13Z

src/library/scala/collection/immutable/Vector.scala

-
-        //println("----- appendBack " + value + " at " + endIndex + " reached block end")
+        val shift = startIndex & ~((1 << 5 * (depth - 1))-1)
+        val shiftBlocks = startIndex >>> 5 * (depth - 1)


Should surround multiplication with parens for clarity

Ichoran · 2016-11-18T19:44:45Z

src/library/scala/collection/immutable/Vector.scala

-    s.cleanLeftEdge(cutIndex-shift)
-    s
-*/
+    val shift = (cutIndex & ~((1 << (5 * d)) - 1))


Don't need outer parens

Ichoran · 2016-11-18T19:45:03Z

src/library/scala/collection/immutable/Vector.scala

@@ -639,13 +572,8 @@ override def companion: GenericCompanion[Vector] = Vector
    val blockIndex = (cutIndex - 1) & ~31
    val xor = startIndex ^ (cutIndex - 1)
    val d = requiredDepth(xor)
-    val shift = (startIndex & ~((1 << (5*d))-1))
+    val shift = (startIndex & ~((1 << (5 * d)) - 1))


Don't need outer parens

Ichoran · 2016-11-18T19:47:31Z

src/library/scala/collection/immutable/Vector.scala

@@ -1123,16 +1041,13 @@ private[immutable] trait VectorPointer[T] {
    }


-
-
    // USED IN APPEND
    // create a new block at the bottom level (and possibly nodes on its path) and prepares for writing

    // requires structure is clean and at pos oldIndex,
    // ensures structure is dirty and at pos newIndex and writable at level 0
    private[immutable] final def gotoFreshPosWritable0(oldIndex: Int, newIndex: Int, xor: Int): Unit = { // goto block start pos
      if (xor < (1 << 5)) { // level = 0


Should leave a comment that we're already at the block start.

Ichoran · 2016-11-18T19:48:34Z

This looks like a welcome improvement overall; just a few aspects that could be even tidier if you're tidying anyway.

paplorinc · 2016-11-18T20:45:04Z

Thanks @Ichoran!
I've applied your suggestions and rebased the commit, hoping the build will pass with this base.
I've also changed all signed left shifts to unsigned ones, since all indices are inherently positive.

paplorinc · 2016-11-18T20:45:26Z

src/library/scala/collection/immutable/Vector.scala

-        display3((index >> 15) & 31) = display2
-        display2((index >> 10) & 31) = display1
-        display1((index >>  5) & 31) = display0
+        display5((index >>> 25) & 31) = display4


indices are inherently positive or zero

If there is error-checking to make sure they're positive or zero, then there is no reason to bother changing. If there is not error-checking, an exception could be replaced with a silently wrong result. I think they should stay signed!

The error checking is in checkRangeConvert and >>> was already used in many other places, it's the reason why I unified it: https://github.com/paplorinc/scala/blob/4279bbc9f467ef6d8a0ddf4190f2dd947f13d580/src/library/scala/collection/immutable/Vector.scala#L482

paplorinc · 2016-11-18T20:46:05Z

src/library/scala/collection/immutable/Vector.scala

-    val a2 = new Array[AnyRef](array.length)
-    java.lang.System.arraycopy(array, 0, a2, 0, right)
-    a2
+  private[immutable] def copyLeft(array: Array[AnyRef], right: Int): Array[AnyRef] = {


to unify with copyOf

paplorinc · 2016-11-18T22:48:08Z

@szeiger, not sure how to fix this unrelated MiMa failure, what should I rebase againts?

paplorinc · 2016-11-19T10:33:35Z

src/library/scala/collection/immutable/Vector.scala

    getElem(idx, idx ^ focus)
  }

  private def checkRangeConvert(index: Int) = {
    val idx = index + startIndex
-    if (0 <= index && idx < endIndex)
+    if (index >= 0 && idx < endIndex)


@Ichoran, this makes sure all indices are strictly non-negative.
(switched the comparison order to make it more intuitive - it's also easier to see that different variables are compared for boundaries)

szeiger · 2016-11-21T12:56:47Z

There's nothing to rebase against yet, all 2.12.x builds are currently broken. I've created #5549 to fix this.

paplorinc · 2016-11-22T14:06:25Z

@szeiger, I still have a MiMa failure (not sure where to view the failure's reason).
Is it because of the deleted final method modifier in the final class?

szeiger · 2016-11-22T14:20:33Z

@paplorinc Go to the details of validate-test, select Console Output and Full Log, search for "binary compatibility":

/home/jenkins/workspace/scala-2.12.x-validate-test/build/pack/lib/scala-library.jar
Found 4 binary incompatibilities (1 were filtered out)
======================================================
 * method debug()Unit in class scala.collection.immutable.VectorBuilder does
   not have a correspondent in current version
 * method debug()Unit in class scala.collection.immutable.VectorIterator
   does not have a correspondent in current version
 * method debug()Unit in class scala.collection.immutable.Vector does not
   have a correspondent in current version
 * method debug()Unit in interface scala.collection.immutable.VectorPointer
   does not have a correspondent in current version

Unlike most projects we require both backwards and forwards binary compatibility for Scala itself, so you can never add or remove a method.

Some cases are really not binary compatible but we're OK with it because it only affects internal code that could not be accessed from outside the standard library without subverting Scala's access restrictions. Package-private elements generally fall into this category. Once you have convinced yourself that the binary-incompatible changes are harmless you can whitelist them. See the log for the ready-to-copy-and-paste whitelisting configuration. Note that it's always shown under backwards compatibility even when it really affects forwards compatibility (and therefore has to go into bincompat-forward-whitelist.conf).

paplorinc · 2016-11-23T12:38:47Z

Thank you @szeiger, had to remove some things, but now it's finally green :/.
@Ichoran, please review :)

Ichoran · 2016-11-23T18:06:15Z

src/library/scala/collection/immutable/Vector.scala

-        val shiftBlocks = startIndex >>> 5*(depth-1)
-
-        //println("----- appendBack " + value + " at " + endIndex + " reached block end")
+        val shift       = startIndex & ~((1 << (5 * (depth - 1))) - 1)


This is not sufficiently similar to the next computation for alignment to be a good idea. It suggests that the two are closely related, when in fact they are in detail rather different computations. So I would just go shift = startIndex without the extra spaces.

Ichoran · 2016-11-23T18:11:22Z

src/library/scala/collection/immutable/Vector.scala

-   with VectorPointer[A @uncheckedVariance] {
+  extends AbstractIterator[A]
+    with Iterator[A]
+    with VectorPointer[A @uncheckedVariance] {


This style makes it hard to tell at a glance where the class signature ends and where the body begins. The style is all over the place in practice, but I recommend left-justifying everything; vertical whitespace should always be used to separate classes so there is no need to indent them to indicate that they all belong to VectorIterator. Failing that, the previous version was preferable because at least all the extended classes/traits are lined up.

Ichoran · 2016-11-23T18:16:30Z

src/library/scala/collection/immutable/Vector.scala

+      } else if (xor < (1 << 25)) { // level = 4
+        display4((index >>> 20) & 31).asInstanceOf[Array[AnyRef]]((index >>> 15) & 31).asInstanceOf[Array[AnyRef]]((index >>> 10) & 31).asInstanceOf[Array[AnyRef]]((index >>>  5) & 31).asInstanceOf[Array[AnyRef]](index & 31).asInstanceOf[T]
+      } else if (xor < (1 << 30)) { // level = 5
+        display5((index >>> 25) & 31).asInstanceOf[Array[AnyRef]]((index >>> 20) & 31).asInstanceOf[Array[AnyRef]]((index >>> 15) & 31).asInstanceOf[Array[AnyRef]]((index >>> 10) & 31).asInstanceOf[Array[AnyRef]]((index >>> 5) & 31).asInstanceOf[Array[AnyRef]](index & 31).asInstanceOf[T]


This is an awful mess even with the improvements unless you have a very wide screen. If you're up for it maybe you can rewrite it in the style

display5((index >>> 25) & 31).asInstanceOf[Array[AnyRef]]. apply((index >>> 20) & 31).asInstanceOf[Array[AnyRef]]. apply((index >>> 15) & 31) . . .

The alternative--lining up the matching part of the displays--pushes the earlier ones so far right that they disappear, so that's probably not a good idea.

how about

var array = display5 array = array((index >>> 25) & 31).asInstanceOf[Array[AnyRef]] array = array((index >>> 20) & 31).asInstanceOf[Array[AnyRef]] array = array((index >>> 15) & 31).asInstanceOf[Array[AnyRef]] array = array((index >>> 10) & 31).asInstanceOf[Array[AnyRef]] array = array((index >>> 5) & 31).asInstanceOf[Array[AnyRef]] array(index & 31).asInstanceOf[T]

or would that be slower than the direct reference?

Ichoran · 2016-11-23T18:18:24Z

src/library/scala/collection/immutable/Vector.scala

-
-}
-
+}


Add a newline at the end to keep things happy that like newlines at the end!

:)) sir, yes sir!

Ichoran · 2016-11-23T23:35:48Z

src/library/scala/collection/immutable/Vector.scala

+        array = array((index >>> 10) & 31).asInstanceOf[Array[AnyRef]]
+        array = array((index >>>  5) & 31).asInstanceOf[Array[AnyRef]]
+        array(index & 31).asInstanceOf[T]
+      } else {                      // level = 6


I have witnessed cases where allocating a temporary var produces worse performance than chaining methods, and at this point in time I think chaining is slightly more idiomatic than repeated assignment to vars. So I'd just chain them as I indicated before. If you don't like the apply, you can do it this way:

array((index >>> 25) & 31). asInstanceOf[Array[AnyRef]]((index >>> 20) & 31). asInstanceOf[Array[AnyRef]]((index >>> 15) & 31). ...

You miss the alignment on the first shift, but I guess you could add whitespace until it was aligned.

Done, resolved it via parenthesizing everything to make it multilinable, making alignment trivial also :)

Ichoran

LGTM

paplorinc · 2016-11-28T17:21:48Z

@retronym, could you please review this also, it's related to https://issues.scala-lang.org/browse/SI-10049 as well?

Unified, since indices are always positive and unsigned shift was already used in other places

paplorinc · 2016-12-06T21:18:29Z

@Ichoran, @retronym, rebased to fix conflict in whitelist.

szeiger · 2016-12-08T15:58:59Z

/nothingtoseehere

The CLA checker had some hickups recently. CLA confirmed.

SethTisue · 2016-12-08T16:51:10Z

thanks @paplorinc! you're killing it lately

paplorinc · 2016-12-09T09:07:26Z

Thank you @Ichoran, @szeiger and @SethTisue! :)

scala-jenkins added this to the 2.12.2 milestone Nov 18, 2016

paplorinc force-pushed the vectorCleanup branch 2 times, most recently from 977ffa5 to 228da21 Compare November 18, 2016 18:13

Ichoran reviewed Nov 18, 2016

View reviewed changes

paplorinc force-pushed the vectorCleanup branch from 228da21 to 4279bbc Compare November 18, 2016 20:43

paplorinc commented Nov 18, 2016

View reviewed changes

paplorinc force-pushed the vectorCleanup branch from 4279bbc to 7bddc2c Compare November 19, 2016 07:41

paplorinc commented Nov 19, 2016

View reviewed changes

paplorinc force-pushed the vectorCleanup branch 2 times, most recently from ff0eae6 to 6ee5e12 Compare November 21, 2016 09:24

paplorinc force-pushed the vectorCleanup branch from 6ee5e12 to 2963129 Compare November 21, 2016 20:02

paplorinc force-pushed the vectorCleanup branch 5 times, most recently from 83fd253 to 009f2fc Compare November 23, 2016 07:42

paplorinc force-pushed the vectorCleanup branch from 009f2fc to 82ed1d1 Compare November 23, 2016 10:10

Ichoran reviewed Nov 23, 2016

View reviewed changes

paplorinc force-pushed the vectorCleanup branch from c97b11b to 1f330ec Compare November 24, 2016 09:27

Ichoran approved these changes Nov 24, 2016

View reviewed changes

paplorinc added 7 commits December 6, 2016 23:14

Removed redundant casts from Vector

f4ae496

Unified masks in Vector

6af55b0

Deleted leftover debug method from Vector

fd338a7

Deleted leftover code-comments from Vector

95a29f2

Applied suggestions to Vector cleanup

5ae77a7

Changed >> to >>> in Vector to unify style

e06d383

Unified, since indices are always positive and unsigned shift was already used in other places

Applied further cleanup to Vector

74db1d3

paplorinc force-pushed the vectorCleanup branch from 1f330ec to 74db1d3 Compare December 6, 2016 21:18

szeiger merged commit 3de1c0c into scala:2.12.x Dec 8, 2016

paplorinc deleted the vectorCleanup branch December 9, 2016 09:05

SethTisue mentioned this pull request Feb 18, 2017

[ci: last-only] merge 2.12.x onto 2.13.x (Feb 17, 2017) #5718

Merged

Minor Vector cleanup #5545

Minor Vector cleanup #5545

Conversation

paplorinc commented Nov 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ichoran commented Nov 18, 2016

paplorinc commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paplorinc commented Nov 18, 2016

paplorinc Nov 19, 2016 • edited

Choose a reason for hiding this comment

szeiger commented Nov 21, 2016

paplorinc commented Nov 22, 2016

szeiger commented Nov 22, 2016

paplorinc commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ichoran Nov 23, 2016 • edited

Choose a reason for hiding this comment

paplorinc Nov 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ichoran left a comment

Choose a reason for hiding this comment

paplorinc commented Nov 28, 2016

paplorinc commented Dec 6, 2016

szeiger commented Dec 8, 2016

SethTisue commented Dec 8, 2016

paplorinc commented Dec 9, 2016

paplorinc commented Nov 18, 2016 •

edited

paplorinc Nov 19, 2016 •

edited

Ichoran Nov 23, 2016 •

edited

paplorinc Nov 23, 2016 •

edited