0

I am still new to Scala, and one of the things I read about is that for-comprehension is equivalent to a flatMap to a certain extent. However, in my code (below), flatMap is taking at least as twice as long to compute. What could be the reason for this?

This is the slow one:

facts.flatMap(f => factActionsMap(f)).filter(_.isValid(facts))

This is the fast equivalent one:

for {
  f <- facts
  a <- factActionsMap(f)
  if a.isValid(facts)
} yield a

factActionsMap is a map between Fact and a Set[Action]. facts is just a Set[Fact].

jbx
  • 21,365
  • 18
  • 90
  • 144
  • how do you compare the performance of the two? – František Hartman Dec 08 '13 at 19:26
  • The benchmarking code + sample inputs would be helpful! – Akos Krivachy Dec 08 '13 at 19:27
  • @frant.hartm I am just getting System.currentTimeMillis() before and after both. – jbx Dec 08 '13 at 19:37
  • @AkosKrivachy Its a bit difficult because its part of a larger program I am trying to debug. The facts set is really small, like 20 or so. The `factActionsMap` is around 150 entries, with the set of each varying between 1 and 400 entries, but only a couple have 400 entries, most lie between the 20 and 60 range. – jbx Dec 08 '13 at 19:43

2 Answers2

8

Let's check the translation:

scala> trait Fact
defined trait Fact

scala> trait Action { def isValid(s: Set[Fact]): Boolean }
defined trait Action

scala> def facts: Set[Fact] = ???
facts: Set[Fact]

scala> def factActionsMap: Map[Fact, Set[Action]] = ???
factActionsMap: Map[Fact,Set[Action]]

scala> import scala.reflect.runtime.{universe => u}
import scala.reflect.runtime.{universe=>u}

scala> val expr = u reify {
     | for {
     |   f <- facts
     |   a <- factActionsMap(f)
     |   if a.isValid(facts)
     | } yield a
     | }
expr: reflect.runtime.universe.Expr[scala.collection.immutable.Set[Action]] = Expr[scala.collection.immutable.Set[Action]]($read.f
acts.flatMap(((f) => $read.factActionsMap.apply(f).withFilter(((a) => a.isValid($read.facts))).map(((a) => a))(Set.canBuildFrom)))
(Set.canBuildFrom))

scala> u show expr.tree
res0: String = $read.facts.flatMap(((f) => $read.factActionsMap.apply(f).withFilter(((a) => a.isValid($read.facts))).map(((a) => a
))(Set.canBuildFrom)))(Set.canBuildFrom)

So, removing REPL stuff (everything starting with $) and the implicit parameters, plus reformatting, we get:

facts.flatMap(f => factActionsMap(f).withFilter(a => a.isValid(facts)).map(a => a))

There are two main differences between this and what you came up with. First, withFilter is applied to fastActionsMap(f) result, whereas you apply to facts.flatMap result. This means flatMap will work over all results, instead of just the ones accepted.

Second, it uses withFilter instead of filter, which avoids creating an extra collection.

Daniel C. Sobral
  • 295,120
  • 86
  • 501
  • 681
  • I understand the intermediate collection being avoided. I didn't understand the `flatMap` working over all the results versus the `for` version. My `for` generator is still going through all the results to filter them out. – jbx Dec 08 '13 at 21:12
  • @jbx Consider that `flatMap` feeds elements of `facts` to a function -- that function being everything else -- and then creates a single collection out of the returned results. Now, if `withFilter` is inside and removes all elements, then the collection being created will get no elements. If `withFilter` is outside, then a collection will be created with all the elements, and then the elements will be removed from the collection. – Daniel C. Sobral Dec 08 '13 at 23:00
1

Unless I'm mistaken, a more precise equivalent to the second expression would be

facts.flatMap(f => factActionsMap(f).withFilter(_.isValid(facts)))

withFilter is like filter, except that is is lazy, so if you're not doing anything with the contents of the Set, you haven't actually applied the filter. You could check this hypothesis by, for example, printing all elements of the set before calling System.currentTimeMillis for the second time.

Thomas Dufour
  • 1,872
  • 1
  • 16
  • 20
  • @DanielC.Sobral Well I admit I omitted `.map(a => a)` after withFilter, but I'd be glad to learn if there's any other mistake in my answer :) – Thomas Dufour Dec 08 '13 at 20:39