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

Comments

Projects
None yet
3 participants
@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

This comment has been minimized.

Copy link
Member

alexandru commented Jun 15, 2016

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

alexandru commented Jun 15, 2016

Yes, we can still drop it.

@alexandru

This comment has been minimized.

Copy link
Member

alexandru commented Jun 15, 2016

@alexandru

This comment has been minimized.

Copy link
Member

alexandru commented Jun 17, 2016

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

This comment has been minimized.

Copy link
Member

alexandru commented Jun 17, 2016

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

alexandru commented Jun 27, 2016

@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

This comment has been minimized.

Copy link
Member

alexandru commented Jun 27, 2016

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

This comment has been minimized.

Copy link
Member

alexandru commented Jun 29, 2016

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

@guersam

This comment has been minimized.

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.

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

#166 Generalize `Task.sequence` for arbitrary collections (#188)
* (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
@hepin1989

This comment has been minimized.

Copy link

hepin1989 commented Sep 9, 2016

should this be close?

@guersam

This comment has been minimized.

Copy link
Contributor Author

guersam commented Sep 10, 2016

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

@guersam guersam closed this Sep 10, 2016

@alexandru

This comment has been minimized.

Copy link
Member

alexandru commented Sep 10, 2016

@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
You can’t perform that action at this time.