This page is no longer maintained — Please continue to the home page at www.scala-lang.org

Stream::flatMap broken (premature optimisation strikes again)

2 replies
Adriaan Moors
Joined: 2009-04-03,
User offline. Last seen 42 years 45 weeks ago.
Hi
I would say the current version of flatMap in Stream is broken. I would like to replace it by the following naive version, as benchmarking shows the optimisation does not bring much (if anything) to the table. 
  def flatMap[A, B, That](f: A => Traversable[B])(implicit bf: CanBuildFrom[Stream[A], B, That]): That     = (if (isEmpty) Stream.Empty       else (f(head).toStream append (tail flatMap f).asInstanceOf[Stream[B]])).asInstanceOf[That]
What do you think? Is my benchmark sound?
Details below.
cheers,adriaan

-8<---8<----8<---8<----8<---8<----8<---8<----8<---8<----8<---8<---
Welcome to Scala version 2.8.0.r19281-b20091026023411 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_15).Type in expressions to have them evaluated.Type :help for more information.
scala> lazy val odds: Stream[Int] = Stream(1) append ( odds flatMap{x => Stream(x + 2)})        odds: Stream[Int] = <lazy>
scala> odds take 1 force res0: scala.collection.immutable.Stream[Int] = Stream(1)
scala> odds take 2 forcejava.lang.StackOverflowError

With the "naive" implementation, all is well:
scala> import scala.collection.Traversableimport scala.collection.Traversable
scala> import scala.collection.generic.CanBuildFromimport scala.collection.generic.CanBuildFrom
scala>   def flatMapStream[A, B, That](self: Stream[A])(f: A => Traversable[B])(implicit bf: CanBuildFrom[Stream[A], B, That]): That       |     = (if (self.isEmpty) Stream.Empty     |        else (f(self.head).toStream append (flatMapStream(self.tail)(f)).asInstanceOf[Stream[B]])).asInstanceOf[That]flatMapStream: [A,B,That](self: Stream[A])(f: (A) => Traversable[B])(implicit bf: scala.collection.generic.CanBuildFrom[Stream[A],B,That])That
scala> lazy val odds: Stream[Int] = Stream(1) append ( flatMapStream(odds) {x => Stream(x + 2)})odds: Stream[Int] = <lazy>
scala> odds take 1 force                                                                                                              res2: scala.collection.immutable.Stream[Int] = Stream(1)
scala> odds take 2 forceres3: scala.collection.immutable.Stream[Int] = Stream(1, 3)
scala> odds take 100 force res4: scala.collection.immutable.Stream[Int] = Stream(1, 3, 5, 7, 9, 11, 13, 15, 17, 19, 21, 23, 25, 27, 29, 31, 33, 35, 37, 39, 41, 43, 45, 47, 49, 51, 53, 55, 57, 59, 61, 63, 65, 67, 69, 71, 73, 75, 77, 79, 81, 83, 85, 87, 89, 91, 93, 95, 97, 99, 101, 103, 105, 107, 109, 111, 113, 115, 117, 119, 121, 123, 125, 127, 129, 131, 133, 135, 137, 139, 141, 143, 145, 147, 149, 151, 153, 155, 157, 159, 161, 163, 165, 167, 169, 171, 173, 175, 177, 179, 181, 183, 185, 187, 189, 191, 193, 195, 197, 199)

So, what went wrong optimising the naive version? (spoiler alert)
  override final def flatMap[B, That](f: A => Traversable[B])(implicit bf: CanBuildFrom[Stream[A], B, That]): That = {    // we assume there is no other builder factory on streams and therefore know that That = Stream[B]     // optimization: drop A's for which f yields no B    var rest = this    var seg: Traversable[B] = null    do {      if (rest.isEmpty) return Stream.Empty.asInstanceOf[That]       seg = f(rest.head)      rest = rest.tail // this forces the tail unnecessarily    } while (seg.isEmpty)     (seg.toStream append (rest flatMap f).asInstanceOf[Stream[B]]).asInstanceOf[That]  }

I would say this optimisation is unnecessary in the first place. I did a little benchmarking with the attached script.
The essence:    lazy val odds: Stream[Int] = Stream(1) append ( flatMapStream(odds) {x => Stream(x + 2)} )    val oddOdds: Stream[Int] = flatMapStream(odds) {x => if(x % 3 == 0 || x % 5 == 0 || x % 7 == 0) Stream() else Stream(x) }      oddOdds take N force // just to exercise the optimisation where the flatMap'ed function returns the empty stream
For an even more extreme case of subsequent empty streams, I also tried this:     val oddOdds: Stream[Int] = flatMapStream(odds) {x => if(x < (N/4).toInt) Stream() else Stream(x) } 
Same numbers...
As you can see in the attached file, I fixed the optimised version (I think...), but I would prefer to revert the optimisation in trunk.
My interpretation of the benchmark results for my machine (specs below) is that none of the versions is significantly better, so I would prefer the simple case. Laziness is tricky enough as it is...

specs: java -versionjava version "1.6.0_15"Java(TM) SE Runtime Environment (build 1.6.0_15-b03-226)Java HotSpot(TM) 64-Bit Server VM (build 14.1-b02-92, mixed mode)
scala -versionScala code runner version 2.8.0.r19281-b20091026023411 -- Copyright 2002-2009, LAMP/EPFL
  Model Identifier: MacBook5,1   Processor Name: Intel Core 2 Duo  Processor Speed: 2.4 GHz  Number Of Processors: 1   Total Number Of Cores: 2  L2 Cache: 3 MB  Memory: 4 GB   Bus Speed: 1.07 GHz
odersky
Joined: 2008-07-29,
User offline. Last seen 45 weeks 6 days ago.
Re: [scala-devel] Stream::flatMap broken (premature optimisation

Hi Adriaan,

You probably need to do some ticket archeology. There were a number of
tickets raised against flatMap. Once was that a long set of items that
all gave an empty traversable should not force a stack overflow. AFAIC
flatMap is what it is to satisfy these tickets with respect to stack
consumption. Performance plays no part in it.

Cheers

Adriaan Moors
Joined: 2009-04-03,
User offline. Last seen 42 years 45 weeks ago.
Re: Re: [scala-devel] Stream::flatMap broken (premature optimi
Hi,
I looked for related tickets, added the missing regressions tests, and fixed the bug for my case (run/stream_flatmap_odds.scala) in http://lampsvn.epfl.ch/trac/scala/changeset/19307/scala/trunk
cheersadriaan
On Mon, Oct 26, 2009 at 8:06 PM, martin odersky <martin.odersky@epfl.ch> wrote:
Hi Adriaan,

You probably need to do some ticket archeology. There were a number of
tickets raised against flatMap. Once was that a long set of items that
all gave an empty traversable should not force a stack overflow. AFAIC
flatMap is what it is to satisfy these tickets with respect to stack
consumption. Performance plays no part in it.

Cheers

Copyright © 2012 École Polytechnique Fédérale de Lausanne (EPFL), Lausanne, Switzerland