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

Make Observable#++'s argument lazy #654

Merged
merged 7 commits into from Oct 5, 2018

Conversation

Projects
None yet
3 participants
@kubukoz
Contributor

kubukoz commented Apr 15, 2018

Fixes #643.

Done as part of the Scala Spree at https://scala.sphere.it/ :D

@Avasil Avasil added the scala-spree label Apr 15, 2018

@kubukoz

This comment has been minimized.

Contributor

kubukoz commented Apr 15, 2018

I see that the build fails when compiling for JS - will take a look

val count = 10000
import concurrent.duration._
assertEquals(nats.take(count).sumL.runSyncUnsafe(1.second), (1 to count).sum)

This comment has been minimized.

@Avasil

Avasil Apr 15, 2018

Collaborator

Using TestScheduler we can manipulate time and instead of runSyncUnsafe use just runAsync and then s.tick(1.second) (test above) - it might also help with passing Scala.JS test :)

This comment has been minimized.

@kubukoz

kubukoz Apr 15, 2018

Contributor

I'm now trying with tick() following the test above's way of doing it :)

@codecov

This comment has been minimized.

codecov bot commented Apr 15, 2018

Codecov Report

Merging #654 into master will decrease coverage by 0.01%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master     #654      +/-   ##
==========================================
- Coverage   90.81%   90.79%   -0.02%     
==========================================
  Files         398      398              
  Lines       11133    11133              
  Branches     2077     2074       -3     
==========================================
- Hits        10110    10108       -2     
- Misses       1023     1025       +2
@Avasil

This comment has been minimized.

Collaborator

Avasil commented Apr 15, 2018

Thanks @kubukoz !
I think we should follow issue #642 with

Also search and replace in the code-base any usage of ++ with concat (I don't think there are any, but should be double checked).

because some operations are using old (strict) ++ (e.g. endWith o :+) and unless we want to change their semantic they should use strict version Observable.concat - let's do that, have this merged and I can make an issue to discuss whether we'd like those operators to be lazy too.

@kubukoz

This comment has been minimized.

Contributor

kubukoz commented Apr 15, 2018

@Avasil gotcha.

The semantics for startWith wouldn't have changed much (self was passed as the rhs, so it would already have been evaluated), but perhaps it'll be more efficient to use concat in it anyways, so I did that

@@ -50,4 +50,15 @@ object RecursiveConcatSuite extends BaseOperatorSuite {
val f = range(0, count).sumL.runAsync; s.tick()
assertEquals(f.value, Some(Success(count.toLong * (count - 1) / 2)))
}
val nats: Observable[Long] = Observable.defer {

This comment has been minimized.

@Avasil

Avasil Apr 15, 2018

Collaborator

Ideally we could just do Observable.now(1L) ++ nats.map(_ + 1) - if Observable.defer is needed then maybe we could just include it in the implementation? So we could even leave old ConcatObservable but pass suspended Observable

This comment has been minimized.

@kubukoz

kubukoz Apr 15, 2018

Contributor

Yeah, I was following the convention from def range above - but I think with ++ having a lazy rhs it doesn't make sense to defer either of these observables. Unless we make range use concat instead too...

This comment has been minimized.

@Avasil

Avasil Apr 15, 2018

Collaborator

I mean that we could leave ConcatObservable strict (and use it in concat implementation if possible - #644) implementing ++ like:

new ConcatObservable(self, Observable.suspend(other))

What do you think?

This comment has been minimized.

@kubukoz

kubukoz Apr 15, 2018

Contributor

it could work - I'll see

This comment has been minimized.

@kubukoz

kubukoz Apr 15, 2018

Contributor

It works, but I'm having some random SOs from time to time - looking for a reason

This comment has been minimized.

@kubukoz

kubukoz Apr 15, 2018

Contributor

Apparently it has something to do with defer not trampolining the function passed to it - but I'm not sure about the details

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Apr 15, 2018

@alexandru There seems to be problem with Observable.++ stack-safety, we've tried different implementations and all of them(using by-name, deferring Observable etc.) were failing due to StackOverflow after running test many times.

}
val nats: Observable[Long] = {
Observable.now(1L) ++ nats.map(_ + 1)

This comment has been minimized.

@alexandru

alexandru Sep 25, 2018

Member

This PR is really old, sorry for coming back so late. I'm not a good PR manager 😐

Anyway, so one problem is that this nats definition is not tail recursive, therefore the application of ++ cannot be. Actually in this case that Observable throws StackOverflowException is a feature, because otherwise you have a memory leak on your hands.

A tail-recursive nats definition would go like this:

val nats: Observable[Long] = {
  def loop(acc: Long): Observable[Long] =
    Observable.now(acc) ++ loop(acc + 1)
  loop(1)
}
@alexandru

This comment has been minimized.

Member

alexandru commented Sep 25, 2018

I changed nats to this, will see if the build passes:

val nats: Observable[Long] = {
  def loop(acc: Long): Observable[Long] =
    Observable.now(acc) ++ loop(acc + 1)
  loop(1)
}
@alexandru

This comment has been minimized.

Member

alexandru commented Sep 25, 2018

A merge with master might be needed.

@alexandru alexandru added this to the 3.0.0 milestone Sep 25, 2018

@alexandru alexandru modified the milestones: 3.0.0, 3.0.0-RC2 Sep 25, 2018

@kubukoz kubukoz force-pushed the kubukoz:observable-plusplus-lazy branch from 0e5d281 to 28f0efd Sep 25, 2018

@kubukoz

This comment has been minimized.

Contributor

kubukoz commented Sep 25, 2018

Rebased to master :) yeah, now I see that mistake with the tail recursion.

@kubukoz

This comment has been minimized.

Contributor

kubukoz commented Sep 25, 2018

Thanks for the commit, btw!

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Sep 25, 2018

Seems like it's just missing

ProblemFilters.exclude[IncompatibleMethTypeProblem]("monix.reactive.Observable.++")
@kubukoz

This comment has been minimized.

Contributor

kubukoz commented Oct 5, 2018

Hopefully that'll be enough ;) thanks @Avasil

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Oct 5, 2018

Thanks and we can finally merge it! @kubukoz

@Avasil Avasil merged commit f4aa085 into monix:master Oct 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment