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

Iterant fold left operators need to handle errors thrown in F[_] context #569

Merged
merged 3 commits into from Jan 26, 2018

Conversation

Projects
None yet
2 participants
@Avasil
Collaborator

Avasil commented Jan 26, 2018

Fixes #563

Avasil added some commits Jan 26, 2018

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Jan 26, 2018

I did benchmark forIterant#toListL to satisfy my curiosity:

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

This comment has been minimized.

codecov bot commented Jan 26, 2018

Codecov Report

Merging #569 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            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
@alexandru

Pretty good @Avasil - see my comments.

@@ -31,37 +33,51 @@ private[tail] object IterantFoldLeft {
final def apply[F[_], S, A](source: Iterant[F, A], seed: => S)(op: (S,A) => S)
(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] = {

This comment has been minimized.

@alexandru

alexandru Jan 26, 2018

Member

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] = {

This comment has been minimized.

@alexandru

alexandru Jan 26, 2018

Member

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(state))
case Suspend(rest, stop) =>
stopRef.elem = stop
rest.flatMap(loop(stopRef)(state))
case Last(a) =>
F.pure(f(state, a) match {

This comment has been minimized.

@alexandru

alexandru Jan 26, 2018

Member

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.

This comment has been minimized.

@Avasil

Avasil Jan 26, 2018

Collaborator

I see, I've fixed it and added tests for this case - please see if I understand desired behavior correctly. :)

rest.flatMap(loop(state))
case Suspend(rest, stop) =>
stopRef.elem = stop
rest.flatMap(loop(stopRef)(state))
case Last(a) =>
f(state, a).map {

This comment has been minimized.

@alexandru

alexandru Jan 26, 2018

Member

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.

@alexandru

This comment has been minimized.

Member

alexandru commented Jan 26, 2018

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 😉

@alexandru alexandru merged commit c7bcecf into monix:master Jan 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Avasil Avasil deleted the Avasil:Iterant#foldL-error-handling branch Sep 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment