Repository navigation
Update to cats-effect 1.0 and use Timer as necessary for IO - #2814
Conversation
moradology
changed the title
echeipesh
left a comment
echeipesh
left a comment
There was a problem hiding this comment.
PR looks good, needs CQs
|
|
||
| val results = (rows flatMap elaborateRow flatMap rowToBytes map retire) | ||
| .join(threads) | ||
| val results = (rows flatMap elaborateRow flatMap rowToBytes flatMap retire) |
There was a problem hiding this comment.
would this be a join(threads) equivalent? i was not sure, that's why decided to make it clear, to keep things simple.
There was a problem hiding this comment.
Or probably it's a 1.0 feature for sure.
There was a problem hiding this comment.
Semantically equivalent, I believe so, but we lose some parallelism here
There was a problem hiding this comment.
We loose some parallelism? What do you mean? You mean that the behaviour would differ significantly?
There was a problem hiding this comment.
@moradology don't you want to use explicit .parJoin(threads) instead of a flatMap on the last step?
There was a problem hiding this comment.
implicit val cs: ContextShift[IO] = IO.contextShift(ec)
(index map readRecord)
.parJoin(threads)
.compile
.toVector
.unsafeRunSync.flattenThere was a problem hiding this comment.
We certainly can, I just wasn't sure people would be pleased with extra implicits flying around without significant/obvious performance wins.
For the sake of argument, why not
((first map second).parJoin(threads) map third).parJoin(threads) where possible?
There was a problem hiding this comment.
@moradology no, it is a question for you, as you introduced this change, i was just asking :D
0c18b7c to
2677f7a
Compare
pomadchin
left a comment
pomadchin
left a comment
There was a problem hiding this comment.
Removal of the join step should be reverted, as it creates a significant performance loss.
All old joins should be replaced with a new parJoin.
| .join(threads) | ||
| val results = rows | ||
| .map(elaborateRow) | ||
| .parJoin(threads) |
There was a problem hiding this comment.
w0000t, i think
(rows flatMap elaborateRow flatMap rowToBytes map retire).parJoin(threads)would be enough! It's already tested and benchmarked long ago, so probably you can just replace all joins with parJoins.
| .join(threads) | ||
| rows | ||
| .map(elaborateRow) | ||
| .parJoin(threads) |
There was a problem hiding this comment.
The same here, no needs in so many parJoins :D
pomadchin
left a comment
pomadchin
left a comment
There was a problem hiding this comment.
LGTM, CQs are remaning. Benchmarks: https://gist.github.com/pomadchin/c286ce342e68d2e2717bf00429475428
4cb1a3d to
a47773d
Compare
There was a problem hiding this comment.
It looks like we need to bump fs2 up to 1.0.0: https://github.com/functional-streams-for-scala/fs2/releases/tag/v1.0.0 and cats up to 1.4.0
fce0693 to
333054d
Compare
* cats-core 1.4.0 * cats-effect 1.0.0 * fs2 1.0.0
echeipesh
changed the title
3 participants
This brings cats-effect to 1.0, which should be faster and have a more stable API going forward.