Add IO#to conversion method #50

alexandru · 2017-05-09T08:53:52Z

This is a follow up on #47

Adding a conversion utility to IO that can convert from any type implementing the Async type-class:

def to[F[_]](implicit F: cats.effect.Async[F]): F[A @uV]

Notes:

internal implementation tries to do conversion as efficient as possible, being one raison for why it exists because doing F.async(io.unsafeRunAsync) can be inefficient, besides being dangerous
we are using @uncheckedVariance to avoid introducing a B >: A type (due to covariance), which would wreck our signature; Scala's standard library also does it for the conversions in the standard collections

codecov-io · 2017-05-09T09:18:14Z

Codecov Report

Merging #50 into master will decrease coverage by 0.37%.
The diff coverage is 75%.

@@            Coverage Diff            @@
##           master     #50      +/-   ##
=========================================
- Coverage   88.27%   87.9%   -0.38%     
=========================================
  Files          17      17              
  Lines         273     281       +8     
  Branches       20      20              
=========================================
+ Hits          241     247       +6     
- Misses         32      34       +2

djspiewak · 2017-05-09T15:47:25Z

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

+
+  override def liftIO[A](ioa: IO[A]): F[A] = {
+    // Implementation for `IO#to` depends on the `Async` type class,
+    // and not on `Effect`, so this shouldn't create a cyclic dependency


It's being overridden anyway.

djspiewak · 2017-05-09T15:49:37Z

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

+        case Pure(a) => F.pure(a)
+        case RaiseError(ex) => F.raiseError(ex)
+        case async =>
+          F.async(async.unsafeRunAsync)


Should keep threading suspends, rather than just immediately jumping to async. It should be possible to convert an entirely synchronous IO in terms of entirely synchronous F constructions, even if the IO involves flatMap and suspend.

I'm not sure what you mean. Doesn't unsafeStep eliminate Suspend and BindSuspend?

From what I've seen that branch is hit only if we have an Async or BindAsync state.

Yeah I just realized you're using unsafeStep. That branch will only be hit at an async point.

I guess it's sort of an open question whether it's better to collapse bind chains or carry them forward into the structure of the target monad. I was originally thinking of the latter implementation, but the former is possible. Both should be lawful.

alexandru · 2017-05-11T13:28:57Z

Removed the WIP status.

Changes:

optimised for Pure and RaiseError - I actually did benchmarking here and the extra pattern matching, while not elegant, makes a big difference for the performance of IO.pure and IO.raiseError, while not decreasing the performance in case of IO.apply or IO.async; I also tried specialisation by overloading that method in Pure and RaiseError, but it's the same thing and I'd rather keep the implementation in the same place
added tests

NOTE - I could not do the stack-safety test generically, because this conversion isn't stack safe unless the source is stack safe

F.async(source.unsafeRunAsync)

And we have a problem that the generator in cats.effect.Generators does not generate stack safe Async instances:

def genAsync[A: Arbitrary]: Gen[IO[A]] = 
  arbitrary[(Either[Throwable, A] => Unit) => Unit].map(IO.async(_))

alexandru · 2017-05-11T13:36:09Z

I have also updated the PR description.

djspiewak

Doesn't surprise me that you can't test the to using the generic generator. The fact that it also generates stack-unsafe IO values was intentional; I wanted to ensure that things were generally safe with IO.async being generated at a moderate probability.

djspiewak · 2017-05-11T16:42:46Z

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

+   * Converts the source `IO` into any `F` type that implements
+   * the [[cats.effect.Async Async]] type class.
+   */
+  final def to[F[_]](implicit F: cats.effect.Async[F]): F[A @uV] =


Minor sidebar: I would rather just write out the whole @uncheckedVariance here. I don't expect to use the annotation elsewhere, so the value of the alias is minimal and IMO not worth the obfuscation.

OK, will fix.

djspiewak · 2017-05-11T16:43:49Z

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

+  final def to[F[_]](implicit F: cats.effect.Async[F]): F[A @uV] =
+    this match {
+      case Pure(a) => F.pure(a)
+      case RaiseError(e) => F.raiseError(e)


👍 This is exactly what I had in mind.

djspiewak · 2017-05-11T16:48:45Z

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

+      case RaiseError(e) => F.raiseError(e)
+      case _ => F.suspend {
+        // Evaluating until finished or the first async boundary
+        unsafeStep match {


Oh actually, randomly, I think we could achieve better stack-safety on the conversion (and push more control into the target Effect) by not using unsafeStep here. Rather, just pattern match the whole thing and pass along the Bind cases to the target. That should in theory mean that the stack properties of the resultant should be identical to the input.

I don't understand what you mean here.

The evaluation of the source IO should still be handled by the IO implementation, because we can't translate this into the target F[_] otherwise:

case BindSuspend(thunk, f) => ???

I mean, besides evaluating our IO with unsafeStep, what else can we do with this?

Something like this:

def to[F[_]: Effect]: F[A @uV] = this match { case Pure(a) => F.pure(a) case RaiseError(e) => F.raiseError(e) case Suspend(thunk) => F.suspend(thunk.to[F]) case Async(k) => F.async(k) case BindSuspend(thunk, f) => F.flatMap(F.suspend(thunk(()).to[F]))(e => f(e).to[F]) case BindAsync(k, f) => F.flatMap(F.async(k))(e => f(e).to[F]) }

The neat thing about this is that ioa.to[IO] === ioa (as in, will have identical structure to). In so far as it is possible, it gives control over the bind chain to the target monad, and ensures stack safety of the output in all cases where the target monad would guarantee stack safety, regardless of IO's own guarantees.

Ah, so this is very elegant, however I think it ends up using more memory than the current version, plus I feel that it is making assumption about how F[_] is implemented, assuming that there's a 1:1 correspondence in those internal states.

The concern here is that this isn't necessarily the case, F[_] adding overhead of its own, while at the same time inheriting IO's own overhead (i.e. you still have AndThen in charge, you still have an AtomicBoolean in that Async(k), etc). I don't have concrete examples right now, this isn't necessarily about Monix's implementation.

If we are converting from IO to F[_] I would rather leave IO's implementation in charge of the actual evaluation.

So besides your implementation being cool 😄 are there any stack safety concerns about the current implementation? Stack safety makes my head hurt, but currently I'm not seeing problems.

My implementation definitely uses some more memory, because unsafeStep is able to discard intermediate things that get captured in in closures by my implementation. One point at a time…

plus I feel that it is making assumption about how F[_] is implemented, assuming that there's a 1:1 correspondence in those internal states.

I'm not sure I see how? It's basically just assuming that F is a lawful effect. In fact, it's actually just assuming that F is a lawful Async, so we could soften the constraints a little further. If F can implement flatMap properly, then there shouldn't be a problem from a semantic standpoint.

Efficiency is the more interesting question. I don't think there's any doubt that the unsafeStep implementation is faster, because it leaves IO maximally in control and essentially forces the target Effect to treat converted actions as black boxes. That removes evaluation control from the target type but does reduce the indirection.

are there any stack safety concerns about the current implementation?

I was thinking there was, since it looked like it was suceptible to the same issue you pointed out on #42, but now I'm not so sure. The only difference between your implementation and mine is that mine preserves the binds into the target type, whereas yours just runs them in-place. That should be exactly the same stack-safety, I think.

The biggest win to my implementation, aside from the fact that it doesn't use any unsafe functions, is the fact that it allows the target monad to redefine what flatMap means between actions in the already-constructed IO. It gives you the flexibility to implement cool things like thread relocation heuristics (e.g. for fairness) and similar, whereas just suspending unsafeStep eliminates that possibility. Basically, my implementation treats IO as a free monad that we're interpreting into some other monad, whereas yours treats it as an opaque box that we can evaluate and suspend, but nothing more.

It gives you the flexibility to implement cool things like thread relocation heuristics (e.g. for fairness) and similar, whereas just suspending unsafeStep eliminates that possibility

You're right here and Monix's Task does just that.

I guess we can go ahead with your version and if we bump into problems down the road, we can backtrack.

Question: do you really want F[_] to have an Effect constraint? Because it seems to me that this works with just an Async.

Question: do you really want F[_] to have an Effect constraint? Because it seems to me that this works with just an Async.

Thinking about that exact question right now. to absolutely can be implemented with just Async. Will add a second reply here within the hour.

@alexandru Ok, so I think we should just constrain to Async. The interesting thing this also implies is that Async should extend LiftIO, rather than deferring it to Effect. There's actually almost no difference between Async and LiftIO, but we'll keep them separate for the time being. There's no particular harm in the two separate representations.

This works out pretty well, because I think #51 shows that Async is a generally very easy typeclass to lift, but Effect is extremely hard. So the more stuff we can usefully constrain solely by Async, the better.

…-safety tests

djspiewak · 2017-05-12T16:56:43Z

laws/shared/src/test/scala/cats/effect/IOTests.scala

+    // Override default generator to only generate
+    // synchronous instances that are stack-safe
+    implicit val arbIO: Arbitrary[IO[Int]] =
+      Arbitrary(Gen.delay(genSyncIO[Int]))


Out of curiosity, with the new implementation, does this work even with the async-including generator?

No, it's the same thing.

Depending on F[_] it might end up being stack safe, but only if F.async would make it so, but we've established that F.async shouldn't do that, which sort of makes sense (I was just complaining that we should have two versions of async in there for the price of 1 :))

Cool, thanks.

djspiewak · 2017-05-12T16:58:02Z

@alexandru I'll move the LiftIO arrow to Async in a separate PR. So I think the only remaining change here would be to change Effect to Async in the constraints. Beyond that, are we all set?

alexandru · 2017-05-12T16:59:04Z

I've already changed the implementation to yours, plus restricted F only to Async.

Yes, Async extends LiftIO should happen in a different PR.

djspiewak · 2017-05-12T17:02:03Z

Tests failed. I'll merge once the build is passing.

alexandru · 2017-05-12T17:12:13Z

Damn, will fix it in about 1 hour.

…

-- Alexandru Nedelcu alexn.org

alexandru · 2017-05-12T18:40:48Z

OK, so I had a faulty generator that was still generating async stuff. I fixed it, works now.

alexandru · 2017-05-12T19:14:05Z

Feel free to merge if you want.

alexandru added 2 commits May 9, 2017 11:49

Add IO#to conversion method

be41c03

Fix compilation

ef3a870

djspiewak reviewed May 9, 2017

View reviewed changes

alexandru added 2 commits May 11, 2017 15:04

Merge branch 'master' into liftIO

af72b60

IO#to optimization, add tests

f7e1d41

alexandru changed the title ~~WIP: Add IO#to conversion method~~ Add IO#to conversion method May 11, 2017

IO#to fix comment

3660c5e

djspiewak approved these changes May 11, 2017

View reviewed changes

djspiewak reviewed May 11, 2017

View reviewed changes

Change implementation to @djspiewak`s suggested one, added more stack…

8dc3cc8

…-safety tests

djspiewak reviewed May 12, 2017

View reviewed changes

djspiewak mentioned this pull request May 12, 2017

Async <: LiftIO #55

Closed

djspiewak modified the milestone: 0.2 May 12, 2017

Fix tests

1822867

djspiewak merged commit 1822867 into typelevel:master May 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IO#to conversion method #50

Add IO#to conversion method #50

alexandru commented May 9, 2017 •

edited

codecov-io commented May 9, 2017 •

edited

djspiewak May 9, 2017

djspiewak May 9, 2017

alexandru May 9, 2017

djspiewak May 9, 2017

alexandru commented May 11, 2017 •

edited

alexandru commented May 11, 2017

djspiewak left a comment

djspiewak May 11, 2017

alexandru May 12, 2017

djspiewak May 11, 2017

djspiewak May 11, 2017

alexandru May 12, 2017

djspiewak May 12, 2017

alexandru May 12, 2017

djspiewak May 12, 2017

alexandru May 12, 2017

djspiewak May 12, 2017

djspiewak May 12, 2017

djspiewak May 12, 2017

alexandru May 12, 2017

djspiewak May 12, 2017

djspiewak commented May 12, 2017

alexandru commented May 12, 2017

djspiewak commented May 12, 2017

alexandru commented May 12, 2017 via email •

edited

alexandru commented May 12, 2017

alexandru commented May 12, 2017

Add IO#to conversion method #50

Add IO#to conversion method #50

Conversation

alexandru commented May 9, 2017 • edited

codecov-io commented May 9, 2017 • edited

Codecov Report

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 11, 2017 • edited

alexandru commented May 11, 2017

djspiewak 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

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

Choose a reason for hiding this comment

djspiewak commented May 12, 2017

alexandru commented May 12, 2017

djspiewak commented May 12, 2017

alexandru commented May 12, 2017 via email • edited

alexandru commented May 12, 2017

alexandru commented May 12, 2017

alexandru commented May 9, 2017 •

edited

codecov-io commented May 9, 2017 •

edited

alexandru commented May 11, 2017 •

edited

alexandru commented May 12, 2017 via email •

edited