Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task Applicative instance doesn't run the tasks in parallel #193

Closed
mmollaverdi opened this Issue Jul 1, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@mmollaverdi
Copy link

mmollaverdi commented Jul 1, 2016

Combining 2 tasks using monix-cat's applicative instance, results in sequential (monadic) execution of those tasks, as opposed to parallel execution which is what you would expect from an applicative.

E.g.

import monix.cats._
implicitly[Applicative[Task]]

val task1 = Task.apply { log("Started Action1"); 1 }.delayResult(5.seconds).map(x=> { log("Finished Action1"); x } )
val task2 = Task.apply { log("Started Action2"); 2 }.map(x=> { log("Finished Action2"); x } )

val task = (task1 |@| task2).map(_ + _)
val result = Await.result(task.runAsync, 10.seconds)

The result I get for a sample run is:

2016-07-01T18:38:35.098 - Started Action1
2016-07-01T18:38:40.113 - Finished Action1
2016-07-01T18:38:40.116 - Started Action2
2016-07-01T18:38:40.117 - Finished Action2

(Task2 only starts once Task1 has finished)

See the full code example here.

If I implement an Applicative instance using Task.mapBoth though, I get the expected results:

object TaskApplicativeInstance {
  implicit val TaskApplicative: Applicative[Task] = new Applicative[Task] {
    override def pure[A](x: A): Task[A] = Task.now(x)

    override def ap[A, B](ff: Task[A => B])(fa: Task[A]): Task[B] = Task.mapBoth(ff, fa)((f, a) => f(a))
  }
}

I get:

2016-07-01T18:44:27.283 - Started Action2
2016-07-01T18:44:27.286 - Started Action1
2016-07-01T18:44:27.286 - Finished Action2
2016-07-01T18:44:32.291 - Finished Action1

See the full code example with this custom applicative instance here.

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 1, 2016

Good point @mmollaverdi, you are looking for the non-deterministic Applicative instance.

@mmollaverdi

This comment has been minimized.

Copy link
Author

mmollaverdi commented Jul 1, 2016

Thanks @dialelo.

Any reason why that's not the default? Also is there a cats applicative version of this?

alexandru added a commit that referenced this issue Jul 1, 2016

@alexandru

This comment has been minimized.

Copy link
Member

alexandru commented Jul 1, 2016

@mmollaverdi @dialelo so normally all you have to do is:

import monix.eval.Task.nondeterminism

Unfortunately there's a problem for Cats. That applicative builder is using product instead of map2 or ap. Which is IMHO a problem with Cats, because product is a redundant operation with map2. And its default implementation is based on flatMap.

I committed a patch that also overrides product to use map2 instead of flatMap. Will happen in the next release.

@mmollaverdi

This comment has been minimized.

Copy link
Author

mmollaverdi commented Jul 1, 2016

Thanks @alexandru. I appreciate your quick response.

@alexandru alexandru added the bug label Jul 22, 2016

@alexandru alexandru added this to the 2.0 milestone Jul 22, 2016

@alexandru alexandru self-assigned this Jul 22, 2016

@mmollaverdi

This comment has been minimized.

Copy link
Author

mmollaverdi commented Aug 3, 2016

I just tested the example I've linked to above with monix 2.0-RC9, but unfortunately got the same result.

@alexandru

This comment has been minimized.

Copy link
Member

alexandru commented Aug 8, 2016

@mmollaverdi are you sure?

There is a test that ensures the tasks are executed in parallel when using import monix.eval.Task.nondeterminism. That test cannot lie.

Also, after running your sample, along with the required import, I got this output:

2016-08-08T09:44:01.819 - Starting
2016-08-08T09:44:01.932 - Started Action2
2016-08-08T09:44:01.933 - Started Action1
2016-08-08T09:44:01.934 - Finished Action2
2016-08-08T09:44:06.944 - Finished Action1
2016-08-08T09:44:06.947 - Result is 3
2016-08-08T09:44:06.947 - Finished

That seems to be running in parallel to me.

Do note that import monix.eval.Task.nondeterminism is required, because parallelism is not the default and there are really good reasons for that, including performance reasons.

@alexandru

This comment has been minimized.

Copy link
Member

alexandru commented Aug 8, 2016

@mmollaverdi btw, sorry for the late response, I've been on vacation :-)

@mmollaverdi

This comment has been minimized.

Copy link
Author

mmollaverdi commented Aug 9, 2016

Hmm, thanks @alexandru. Adding import monix.eval.Task.nondeterminism fixed it for me as you suggested.

btw, sorry for the late response, I've been on vacation :-)

No worries at all. I hope you had a good time and thanks for your help.

@alexandru

This comment has been minimized.

Copy link
Member

alexandru commented Aug 9, 2016

Cool. Closing the issue for now.

@alexandru alexandru closed this Aug 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.