Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collections: remove redundant calls to .seq #3089

Merged
merged 1 commit into from Nov 3, 2013
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/library/scala/collection/IterableLike.scala
Expand Up @@ -202,7 +202,7 @@ self =>
b.sizeHintBounded(n, this)
val lead = this.iterator drop n
var go = false
for (x <- this.seq) {
for (x <- this) {
if (lead.hasNext) lead.next()
else go = true
if (go) b += x
Expand Down
6 changes: 3 additions & 3 deletions src/library/scala/collection/SeqLike.scala
Expand Up @@ -273,7 +273,7 @@ trait SeqLike[+A, +Repr] extends Any with IterableLike[A, Repr] with GenSeqLike[

def reverseMap[B, That](f: A => B)(implicit bf: CanBuildFrom[Repr, B, That]): That = {
var xs: List[A] = List()
for (x <- this.seq)
for (x <- this)
xs = x :: xs
val b = bf(repr)
for (x <- xs)
Expand Down Expand Up @@ -478,7 +478,7 @@ trait SeqLike[+A, +Repr] extends Any with IterableLike[A, Repr] with GenSeqLike[

private def occCounts[B](sq: Seq[B]): mutable.Map[B, Int] = {
val occ = new mutable.HashMap[B, Int] { override def default(k: B) = 0 }
for (y <- sq.seq) occ(y) += 1
for (y <- sq) occ(y) += 1
occ
}

Expand Down Expand Up @@ -608,7 +608,7 @@ trait SeqLike[+A, +Repr] extends Any with IterableLike[A, Repr] with GenSeqLike[
val len = this.length
val arr = new ArraySeq[A](len)
var i = 0
for (x <- this.seq) {
for (x <- this) {
arr(i) = x
i += 1
}
Expand Down
4 changes: 2 additions & 2 deletions src/library/scala/collection/TraversableLike.scala
Expand Up @@ -481,7 +481,7 @@ trait TraversableLike[+A, +Repr] extends Any
var follow = false
val b = newBuilder
b.sizeHint(this, -1)
for (x <- this.seq) {
for (x <- this) {
if (follow) b += lst
else follow = true
lst = x
Expand All @@ -506,7 +506,7 @@ trait TraversableLike[+A, +Repr] extends Any
private[this] def sliceInternal(from: Int, until: Int, b: Builder[A, Repr]): Repr = {
var i = 0
breakable {
for (x <- this.seq) {
for (x <- this) {
if (i >= from) b += x
i += 1
if (i >= until) break
Expand Down
4 changes: 2 additions & 2 deletions src/library/scala/collection/TraversableOnce.scala
Expand Up @@ -97,7 +97,7 @@ trait TraversableOnce[+A] extends Any with GenTraversableOnce[A] {
// for internal use
protected[this] def reversed = {
var elems: List[A] = Nil
self.seq foreach (elems ::= _)
self foreach (elems ::= _)
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should work as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa, whoa, that won't work without seq, will it? elems isn't
synchronized. If you traverse in parallel you'll end up with missed
elements.

Where are these pull requests?

--Rex

On Thu, Oct 31, 2013 at 4:52 AM, Aleksandar Prokopec <
notifications@github.com> wrote:

In src/library/scala/collection/TraversableOnce.scala:

@@ -97,7 +97,7 @@ trait TraversableOnce[+A] extends Any with GenTraversableOnce[A] {
// for internal use
protected[this] def reversed = {
var elems: List[A] = Nil

  • self.seq foreach (elems ::= _)
  • self foreach (elems ::= _)

Seems like this should work as well.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3089/files#r7339648
.

Copy link
Member Author

Choose a reason for hiding this comment

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

We know that self is already TraversableOnce, not a GenTraversableOnce.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, okay. Fair enough then; I agree there's no need for .seq.

--Rex

On Thu, Oct 31, 2013 at 7:53 AM, Jason Zaugg notifications@github.comwrote:

In src/library/scala/collection/TraversableOnce.scala:

@@ -97,7 +97,7 @@ trait TraversableOnce[+A] extends Any with GenTraversableOnce[A] {
// for internal use
protected[this] def reversed = {
var elems: List[A] = Nil

  • self.seq foreach (elems ::= _)
  • self foreach (elems ::= _)

We know that self is already TraversableOnce, not a GenTraversableOnce.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3089/files#r7343601
.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Ichoran

The other pull requests will be trickling in. I've been throwing a lot of ideas around in https://github.com/retronym/scala/compare/topic/opt2 and will be working to find what's worthwhile in there with @gkossakowski next week.

On that branch, I saw improvements in the order of 10-15% of a warmed up resident compiler FSC repeatedly processing src/library/util/**.scala. But the usual caveats apply; that result might not generalize, and I might have made things incorrect along the way.

Some (many) of the commits there seemed to deliver cleaner YourKit profiles without real improvements in elapsed time.

Some profile results suprised me (as usual). A lot of time was reported in the constructors of AbstractSeq and friends. I ended up writing a custom ListBuffer that didn't participate in the collections hierarchy at all to, and use that in a custom version of map that was specialized for List (no allocations on Nil.map, no indirection through the CanBuildFrom).

As soon as we figure out which changes seem to be the most beneficial, I'll let you know.

elems
}

Expand Down Expand Up @@ -140,7 +140,7 @@ trait TraversableOnce[+A] extends Any with GenTraversableOnce[A] {

def foldLeft[B](z: B)(op: (B, A) => B): B = {
var result = z
this.seq foreach (x => result = op(result, x))
this foreach (x => result = op(result, x))
result
}

Expand Down
2 changes: 1 addition & 1 deletion src/library/scala/collection/generic/Growable.scala
Expand Up @@ -54,7 +54,7 @@ trait Growable[-A] extends Clearable {
loop(xs.tail)
}
}
xs.seq match {
xs match {
case xs: scala.collection.LinearSeq[_] => loop(xs)
case xs => xs foreach +=
}
Expand Down
2 changes: 1 addition & 1 deletion src/library/scala/collection/generic/Shrinkable.scala
Expand Up @@ -46,5 +46,5 @@ trait Shrinkable[-A] {
* @param xs the iterator producing the elements to remove.
* @return the $coll itself
*/
def --=(xs: TraversableOnce[A]): this.type = { xs.seq foreach -= ; this }
def --=(xs: TraversableOnce[A]): this.type = { xs foreach -= ; this }
}
2 changes: 1 addition & 1 deletion src/library/scala/collection/immutable/DefaultMap.scala
Expand Up @@ -46,7 +46,7 @@ trait DefaultMap[A, +B] extends Map[A, B] { self =>
*/
override def - (key: A): Map[A, B] = {
val b = newBuilder
for (kv <- this.seq ; if kv._1 != key) b += kv
for (kv <- this ; if kv._1 != key) b += kv
b.result()
}
}
2 changes: 1 addition & 1 deletion src/library/scala/collection/mutable/ArrayStack.scala
Expand Up @@ -157,7 +157,7 @@ extends AbstractSeq[T]
* @param xs The source of elements to push.
* @return A reference to this stack.
*/
override def ++=(xs: TraversableOnce[T]): this.type = { xs.seq foreach += ; this }
override def ++=(xs: TraversableOnce[T]): this.type = { xs foreach += ; this }

/** Does the same as `push`, but returns the updated stack.
*
Expand Down
2 changes: 1 addition & 1 deletion src/library/scala/collection/mutable/Stack.scala
Expand Up @@ -119,7 +119,7 @@ extends AbstractSeq[A]
* @param xs the traversable object.
* @return the stack with the new elements on top.
*/
def pushAll(xs: TraversableOnce[A]): this.type = { xs.seq foreach push ; this }
def pushAll(xs: TraversableOnce[A]): this.type = { xs foreach push ; this }

/** Returns the top element of the stack. This method will not remove
* the element from the stack. An error is signaled if there is no
Expand Down