Change IO's run-loop to encode error handlers #46

alexandru · 2017-05-06T08:29:04Z

I discovered a weird space leak that I talked about in #42 (comment) by debugging the behavior of this law:

  lazy val stackSafetyOnRepeatedAttempts = {
    val result = (0 until 10000).foldLeft(F.delay(())) { (acc, _) =>
      F.attempt(acc).map(_ => ())
    }

    F.runAsync(result)(_ => IO.unit).unsafeRunSync() <-> (())
  }

This PR fixes this space leak by changing the behavior of IO#attempt:

removes the AndThen.ShortCircuit logic
adds the notion of AndThen.ErrorHandler, which is meant to process errors
upon an error being signaled, in the AndThen implementation we discard Single references until we reach an ErrorHandler that can process our error, or until we've got no more AndThen nodes to process, in which case we rethrow the error ... this is basically how Java's call-stack also works

This implementation also opens up the possibility of a more optimized handleErrorWith, since now we don't necessarily need attempt.flatMap to express it.

codecov-io · 2017-05-06T08:34:36Z

Codecov Report

Merging #46 into master will increase coverage by 0.97%.
The diff coverage is 97.5%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   86.94%   87.91%   +0.97%     
==========================================
  Files          17       17              
  Lines         268      273       +5     
  Branches       10       20      +10     
==========================================
+ Hits          233      240       +7     
+ Misses         35       33       -2

djspiewak

This looks really fantastic. Would you mind adding a couple more AndThenTests though? I think the test coverage here is probably alright, but it's through IO. If only for documentation reasons, having some of these edge cases tested directly on AndThen would be nice.

Very clever trick though. Obvious in hindsight.

djspiewak · 2017-05-07T18:05:55Z

core/shared/src/main/scala/cats/effect/internals/AndThen.scala

    var continue = true
+
+    @inline def processRight(f: (Any) => Any): Unit =


This is probably actually faster if we don't inline in the bytecode, and HotSpot is allowed to do its thing due to the byte size of the body.

alexandru · 2017-05-07T19:00:06Z

@djspiewak will do, tomorrow :-)

alexandru · 2017-05-08T09:10:37Z

Changes:

have added more tests in AndThenTests, bringing coverage to 100% :-P
- btw: those tests specified with forAll were not running, for ScalaCheck you have to use check, see change
removed those two @inline annotations

djspiewak · 2017-05-08T16:47:03Z

core/shared/src/main/scala/cats/effect/internals/AndThen.scala

@@ -44,14 +44,14 @@ private[effect] sealed abstract class AndThen[-A, +B] extends Product with Seria
    var hasSuccessRef = isSuccess
    var continue = true

-    def processRight(f: (Any) => Any): Unit =
+    def processSuccess(f: (Any) => Any): Unit =


👍👍👍

djspiewak · 2017-05-08T16:48:08Z

I'm 👍 on this, but I want to hear from @mpilquist before merging. If he gives it the thumbs up, I'll bring it in. I'm pretty excited about this too, since I suspect it'll make Effect[StateT[F, S, ?]] (given Effect[F] and Monoid[S]) possible, whereas before the laws would blow the heap.

mpilquist

👍 Looks great!

mpilquist · 2017-05-08T23:09:18Z

core/shared/src/main/scala/cats/effect/IO.scala

@@ -135,7 +141,17 @@ sealed abstract class IO[+A] {
   *
   * @see [[IO.raiseError]]
   */
-  def attempt: IO[Either[Throwable, A]]
+  def attempt: IO[Either[Throwable, A]] = {
+    val fe = AndThen((a: A) => Pure(Right(a)), e => Pure(Left(e)))


Avoid constructing this in Pure and RaiseError cases? E.g., change to def

What do you mean?

Change val fe = ... to def fe = ... to avoid constructing a new AndThen and then not using it in the Pure and RaiseError cases below.

Oh right! Github cut off the diff so I couldn't see. I'll make that change on merge.

mpilquist · 2017-05-08T23:18:49Z

core/shared/src/test/scala/cats/effect/internals/AndThenTests.scala


-class AndThenTests extends FunSuite with Matchers {
+class AndThenTests extends FunSuite with Matchers with Checkers {


Minor thing: I prefer GeneratorDrivenPropertyChecks and using matchers in forAll instead of switching to the boolean assertion style. Not suggesting changing this though.

Oh is that how you use matchers in properties? I was wondering. I miss Specs2. :-(

alexandru changed the title ~~Change IO`s run-loop implementation to encode error handlers~~ Change IO's run-loop implementation to encode error handlers May 6, 2017

alexandru changed the title ~~Change IO's run-loop implementation to encode error handlers~~ Change IO's run-loop to encode error handlers May 6, 2017

alexandru mentioned this pull request May 6, 2017

Effect.stackSafetyOnRepeatedAttempts is too strong #42

Closed

Change IO`s run-loop implementation to encode error handlers

1709760

alexandru force-pushed the attempt branch from c6d8349 to 1709760 Compare May 6, 2017 08:34

alexandru requested review from mpilquist and djspiewak May 6, 2017 08:41

djspiewak reviewed May 7, 2017

View reviewed changes

Add more tests for AndThen

5c48a83

Internal renames for consistency

17dabc4

djspiewak reviewed May 8, 2017

View reviewed changes

djspiewak approved these changes May 8, 2017

View reviewed changes

mpilquist approved these changes May 8, 2017

View reviewed changes

djspiewak merged commit 17dabc4 into typelevel:master May 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change IO's run-loop to encode error handlers #46

Change IO's run-loop to encode error handlers #46

alexandru commented May 6, 2017 •

edited

codecov-io commented May 6, 2017 •

edited

djspiewak left a comment

djspiewak May 7, 2017

alexandru commented May 7, 2017

alexandru commented May 8, 2017

djspiewak May 8, 2017

djspiewak commented May 8, 2017

mpilquist left a comment

mpilquist May 8, 2017

djspiewak May 8, 2017

mpilquist May 8, 2017

djspiewak May 8, 2017

mpilquist May 8, 2017

djspiewak May 8, 2017

		var continue = true

		@inline def processRight(f: (Any) => Any): Unit =


		class AndThenTests extends FunSuite with Matchers {
		class AndThenTests extends FunSuite with Matchers with Checkers {

Change IO's run-loop to encode error handlers #46

Change IO's run-loop to encode error handlers #46

Conversation

alexandru commented May 6, 2017 • edited

codecov-io commented May 6, 2017 • edited

Codecov Report

djspiewak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandru commented May 7, 2017

alexandru commented May 8, 2017

Choose a reason for hiding this comment

djspiewak commented May 8, 2017

mpilquist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandru commented May 6, 2017 •

edited

codecov-io commented May 6, 2017 •

edited