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

Generalize Task.sequence for arbitrary collections #166

Closed
guersam opened this issue Jun 15, 2016 · 16 comments
Closed

Generalize Task.sequence for arbitrary collections #166

guersam opened this issue Jun 15, 2016 · 16 comments
Milestone

Comments

@guersam
Copy link
Contributor

guersam commented Jun 15, 2016

Task.sequence takes Seq[Task[A]] and gives back Task[List[A]], which might not be desirable in all cases.

Generalizing it as def sequence[A, M[X] <: TraversableOnce[X]: CanBuildFrom](xs: M[Task[A]]): Task[M[A]] would be helpful, as well as adding traverse: M[A] => (A => Task[B]) => Task[M[A]] in the same way.

@alexandru
Copy link
Member

I don't dislike the idea. I'm generally fearful of CanBuildFrom, but it's indeed useful.

But if we do this, then gather and gatherUnordered and zipList also need it, for consistency.

@guersam
Copy link
Contributor Author

guersam commented Jun 15, 2016

You're right. Then should we keep zipList, although it doesn't always produce a list? I'd suggest just dropping it IMHO, because it's just an alias and the word 'zip' is sometimes confusing.

@alexandru
Copy link
Member

Yes, we can still drop it.

@alexandru
Copy link
Member

@guersam also Coeval.sequence :-)

@alexandru
Copy link
Member

Note - I think zipList should not ne removed after all. And signature should stay the same. This is for consistency with Observable (I remembered the reason for why it's there :-))

@alexandru
Copy link
Member

I mean, on Observablr the zip operation is not like map2 or mapBoth. And zipList makes sense for Observable, whereas sequence and gather do not.

@guersam
Copy link
Contributor Author

guersam commented Jun 23, 2016

  1. Ok, then how about making Task.zipList receive a vararg as the same as Observable.zipList?
  2. Rewriting gatherUnordered using mutable builder brings a lock which might not be desirable. It seems fine to keep it unchanged by itself, but it causes signature inconsistency between gather and gatherUnordered. We need to choose between more flexible gather and consistency between similar functions. What do you prefer?

@alexandru
Copy link
Member

alexandru commented Jun 23, 2016

@guersam

  1. I agree.
  2. I did some benchmarks lately and using Atomic is cool, but combining Atomic with persistent data-structures (like Vector) destroys performance as Vector is terrible - it's non-blocking, sure, but the performance loss isn't worth it. Just work with synchronize and make the signatures similar.

@guersam
Copy link
Contributor Author

guersam commented Jun 24, 2016

As the order doesn't matter, how about replacing Vector with List? Prepending to a list is significantly faster than appending to a vector. Then we can convert the final list to the desired container using CanBuildFrom or not.

In this case, it would be better to find another way to determine the end of computation not relying on .length, which takes O(n) on lists.

@alexandru
Copy link
Member

@guersam sorry for the late response, I'm caught up with other things and have high latency these days.

I do not want us to use another chunk of O(n) memory at the end, just to convert to a desired type. So we have two choices:

  1. we synchronize on the CanBuildFrom builder
  2. we use a List and return it just as it is - which means that results will happen mostly in reverse order, with non-deterministic variations of course - but given that this is gatherUnordered we are talking about, I think that's totally fine

Frankly, I'd go with number 2. This is because people would use gatherUnordered for performance reasons. And we could keep the Atomic which is cool for the non-blocking behavior. But then I'd do a benchmark to validate the assumption that number 2 is better or at least on par with number 1, because the last benchmark I did left me with a sour taste about using persistent vectors in atomic refs. In the benchmarks subproject you already have the setup and it's easy to try it out.

@alexandru
Copy link
Member

Btw - if you want to work on this, don't wait for me to approve - like do something, submit a PR and then we can also talk on the PR. When you've got time of course.

@alexandru alexandru added this to the 2.0 milestone Jun 29, 2016
@alexandru
Copy link
Member

@guersam - I want to release this as part of release 2.0. Do you have time to work on it?

@guersam
Copy link
Contributor Author

guersam commented Jun 29, 2016

I've modified the code itself, but additional tests and docs are not done yet. Will open a PR soon with current changes to discuss remains.

guersam added a commit to guersam/monix that referenced this issue Jun 29, 2016
guersam added a commit to guersam/monix that referenced this issue Jul 21, 2016
guersam added a commit to guersam/monix that referenced this issue Jul 22, 2016
alexandru pushed a commit that referenced this issue Jul 22, 2016
* (WIP) #166 Generalize `Task.sequence` for arbitrary collections
* `{Task,Coeval}.zipList` takes varargs to match `Observable.zipList`
* Add `Task.traverse`
* Increase GC timeout for tck spec 313 in Travis CI
* Add guersam to AUTHORS
* Update SBT to 0.13.12
* Increase timeout for TCK spec 313
* Ensure that Task.gatherUnordered runs over an iterator
* Replace hardcoded main scala version with Travis config
@He-Pin
Copy link

He-Pin commented Sep 9, 2016

should this be close?

@guersam
Copy link
Contributor Author

guersam commented Sep 10, 2016

Oh, it wasn't automatically closed... thanks for pointing it out.

@guersam guersam closed this as completed Sep 10, 2016
@alexandru
Copy link
Member

@guersam @hepin1989 yes, it's deployed in 2.0.0, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants