- About Scala
- Documentation
- Code Examples
- Software
- Scala Developers
Stream::flatMap broken (premature optimisation strikes again)
Mon, 2009-10-26, 20:02
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
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
Tue, 2009-10-27, 11:37
#2
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:
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
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