Tail recursion issue

Question

We were experimenting with parallel collections in Scala and wanted to check whether the result was ordered. For that, I wrote a small function on the REPL to do that check on the very large List we were producing:

def isOrdered(l:List[Int]):Boolean = { l match { 
  case Nil => true
  case x::Nil => true
  case x::y::Nil => x>y
  case x::y::tail => x>y & isOrdered(tail) 
  }
}

It fails with a stackOverflow (how appropriate for a question here!). I was expecting it to be tail-optimized. What's wrong?

Actually this will yield `true` if the head element is bigger than the following one, etc. Like in 3,2,1. — Pablo Fernandez, Oct 19 '11 at 14:23
Not directly related to your question, but note that your `case x::y::Nil` is superfluous, and your `case x::y::tail` is buggy (consider `List(3,2,9)`). — dave4420, Oct 19 '11 at 14:33

score 14 · Accepted Answer · answered Oct 19 '11 at 14:13

14

isOrdered is not the last call in your code, the & operator is. Try this instead:

@scala.annotation.tailrec def isOrdered(l:List[Int]):Boolean = { l match { 
  case Nil => true
  case x::Nil => true
  case x::y::Nil => x>y
  case x::y::tail => if (x>y) isOrdered(tail) else false
  }
}

answered Oct 19 '11 at 14:13

Kim Stebel

41,826
12
125
142

1

Reference for future readers: Although this answers the question correctly on the *why* the algo is not tail-optimized, note that, as others correctly mentioned, the algorithm is incorrect. @Luigi Plinge answer although does not touch on the key element of the question, contains the corrected version of this algorithm. – maasg Oct 21 '11 at 16:07
True. I didn't check the algorithm at all. – Kim Stebel Oct 21 '11 at 17:18
1

@maasg Funnily enough, despite 14 upvotes, this answer is incomplete: if you take your original version and put @tailrec in front of it and change the bitwise operator `&` to the correct version, `&&`, the compiler is able to make it tail recursive. So it's a problem with the operator used rather than its position. – Luigi Plinge Oct 22 '11 at 09:50
@Luigi Plinge would you update your answer with this info + short explanation on why it works? Would like to have a good,complete answer for this q. That's the idea of SO, isn't it? – maasg Oct 24 '11 at 10:56
@maasg I'm guessing but *think* the compiler can do tail-recursion with `&&` only because it's a short-circuit operator. If you think about it, it doesn't need to keep the previous stack frame, because if the RHS is evaluated then the return value only depends on the RHS. – Luigi Plinge Oct 25 '11 at 04:51

Luigi Plinge · Answer 2 · 2011-10-19T19:50:13.153

8

Your algorithm is incorrect. Even with @Kim's improvement, isOrdered(List(4,3,5,4)) returns true.

Try this:

def isOrdered(l:List[Int]): Boolean = l match {
  case Nil => true
  case x :: Nil => true
  case x :: y :: t => if (x <= y) isOrdered(l.tail) else false
}

(also updated so that signs are correct)

edit: my perferred layout would be this:

def isOrdered(list: List[Int]): Boolean = list match {
  case Nil      => true
  case x :: Nil => true
  case x :: xs  => if (x > xs.head) false
                   else isOrdered(xs)
}

The quick way if performance isn't a problem would be

def isOrdered(l: List[Int]) = l == l.sorted

edited Oct 19 '11 at 19:50

answered Oct 19 '11 at 14:57

Luigi Plinge

50,650
20
113
180

I see the flaw in my algo. Thanks. I guess you can also do `isOrdered(y::t)` to keep the expression in the ctx of the case. – maasg Oct 19 '11 at 19:23
2

@maasg Don't do that: execution time will triple. You could instead write `case x :: t => if (x <= t.head) isOrdered(t) else false` which is equivalent to the above and avoids introducing the extra `y` variable. – Luigi Plinge Oct 19 '11 at 19:29
Interesting and surprisingly close. I measured x2.6 slower execution. Would you care to mention why? cons is supposed to be O(1) – maasg Oct 19 '11 at 20:29
@maasg I'm a bit hazy on the details but I know `head` and `tail` are free: they just return the value of `hd` and `tl` which are private fields of the case class `::`. When you start taking other sublists I imagine new case class instances need to be created. This is a fixed overhead per recursion, but if you do an O(1) operation n times the overall effect is O(n). – Luigi Plinge Oct 19 '11 at 21:04
1

I kept thinking on this one and found what I think is a decent idiomatic solution: `list.foldLeft((true,list.head))((x,y)=>(x._1 && x._2 – maasg Oct 20 '11 at 16:46

score 2 · Answer 3 · answered Oct 19 '11 at 14:11

It can't be tail-optimized because you return this: 'x>y & isOrdered(tail)'. It means it will need to keep it on the stack.

Use the @tailrec annotation to force an error when you expect functions to be tail-recursive. It will also explain why it can't be.

score 1 · Answer 4 · answered Oct 19 '11 at 14:12

I think the problem is that you're using the bitwise-and operator (&) in your last case. Since the runtime needs to know the value of the isOrdered call before it can evaluate the &, it can't tail-optimize the function. (That is, there is more code to run--the bitwise-and operation--after isOrdered is called.)

Using && or an if statement may help.

Tail recursion issue

4 Answers4

Linked