Repository navigation
Iterant fold left operators need to handle errors thrown in F[_] context - #569
Conversation
|
I did benchmark for Before:
[info] Benchmark Mode Cnt Score Error Units
[info] IterantFoldLeftLBenchmark.toListL100 thrpt 100 391092.509 ± 3795.786 ops/s
[info] IterantFoldLeftLBenchmark.toListL10000 thrpt 100 4020.913 ± 30.503 ops/s
[info] IterantFoldLeftLBenchmark.toListL100000 thrpt 100 425.652 ± 1.370 ops/s
After:
[info] Benchmark Mode Cnt Score Error Units
[info] IterantFoldLeftLBenchmark.toListL100 thrpt 100 361662.963 ± 4809.333 ops/s
[info] IterantFoldLeftLBenchmark.toListL10000 thrpt 100 3952.470 ± 48.976 ops/s
[info] IterantFoldLeftLBenchmark.toListL100000 thrpt 100 376.399 ± 3.289 ops/s |
Codecov Report
@@ Coverage Diff @@
## master #569 +/- ##
==========================================
+ Coverage 90.56% 90.61% +0.04%
==========================================
Files 369 369
Lines 9782 9801 +19
Branches 1858 1839 -19
==========================================
+ Hits 8859 8881 +22
+ Misses 923 920 -3 |
| (implicit F: Sync[F]): F[S] = { | ||
|
|
||
| def loop(self: Iterant[F, A], state: S): F[S] = { | ||
| def loop(stopRef: ObjectRef[F[Unit]])(self: Iterant[F, A], state: S): F[S] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets do currying here. Put the state on the left, such that you can then do ...
rest.flatMap(loop(stopRef, newState))| } | ||
|
|
||
| def loop(state: S)(self: Iterant[F, A]): F[S] = { | ||
| def loop(stopRef: ObjectRef[F[Unit]])(state: S)(self: Iterant[F, A]): F[S] = { |
There was a problem hiding this comment.
We don't need 3 parameter groups for this curried function. stopRef and state can be on the same level so to speak.
| rest.flatMap(loop(stopRef)(state)) | ||
|
|
||
| case Last(a) => | ||
| F.pure(f(state, a) match { |
There was a problem hiding this comment.
Calling f can throw, which can trigger an exception that's going to trigger our handleErrorWith, which is going to call an earlyStop reference that's no longer in service.
Do stopRef.elem = null.
There was a problem hiding this comment.
I see, I've fixed it and added tests for this case - please see if I understand desired behavior correctly. :)
| rest.flatMap(loop(stopRef)(state)) | ||
|
|
||
| case Last(a) => | ||
| f(state, a).map { |
There was a problem hiding this comment.
Same problem here. stopRef.elem = null. If unsure then it's easier to think that this assignment needs to happen on all branches, unless you can reason about why it isn't needed.
|
PS: benchmark doesn't look bad, the difference seems to be within the error margin. Our ugly var is working, without this trick throughput wouldn't have been half of what it is 😉 |
Fixes #563