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

Lazy evaluation: good or bad? #465

Closed
raimohanska opened this Issue Nov 12, 2014 · 14 comments

Comments

Projects
None yet
8 participants
@raimohanska
Contributor

raimohanska commented Nov 12, 2014

The fact that bacon.js evaluates event values lazily is a bit confusing. This bit me in my Oredev presentation. So if I couldn't take that into account, how many new Bacon.js users will?

Now, if you do

    fieldValue = $(".foo").asEventStream('keyup').map((e) => $(e.currentTarget).val()).toProperty()
    firstClick = clicks.take(1)
    secondClick = clicks.skip(1).take(2)
    twoValues = Bacon.combineAsArray(
      fieldValue.sampledBy(firstClick),
      fieldValue.sampledBy(secondClick)
    )
    twoValues.log()

What do you get? An array of the values of the .foo textfield at the times of first and second click? Well, not exactly. You'll get the value of the textfield at the time of the second click, twice.

This is because the value of the events isn't evaluated until it's actually used. (see also https://github.com/baconjs/bacon.js/#lazy-evaluation)

Now the question is, should we remove lazy evaluation altogether in Bacon.js 0.8? My suggestion would be to make a spike and see if that brings any serious performance penalty. Then proceed accordingly.

@rpominov

This comment has been minimized.

Show comment
Hide comment
@rpominov

rpominov Nov 12, 2014

If an operation always happens eventually, without laziness performance should be better. But if some operations never applied, for example in a.map(heavyWork).throttle(1000), it depends on what user do in heavyWork. So I think it would be tricky to estimate performance penalty here.

If an operation always happens eventually, without laziness performance should be better. But if some operations never applied, for example in a.map(heavyWork).throttle(1000), it depends on what user do in heavyWork. So I think it would be tricky to estimate performance penalty here.

@phadej

This comment has been minimized.

Show comment
Hide comment
@phadej

phadej Nov 12, 2014

Member

@pozadi yet your example could be done with few helpers:

lazy = (f) -> (x) -> () -> f(x)
force = (thunk) -> thunk()

$out = $in.map(lazy(heavywork)).throttle(1000).map(force)
Member

phadej commented Nov 12, 2014

@pozadi yet your example could be done with few helpers:

lazy = (f) -> (x) -> () -> f(x)
force = (thunk) -> thunk()

$out = $in.map(lazy(heavywork)).throttle(1000).map(force)
@phadej

This comment has been minimized.

Show comment
Hide comment
@phadej

phadej Nov 12, 2014

Member

I.e. If I have to choose I'd pick eager evaluation, because as a user I can add lazyness, but cannot remove it.

Member

phadej commented Nov 12, 2014

I.e. If I have to choose I'd pick eager evaluation, because as a user I can add lazyness, but cannot remove it.

@lautis

This comment has been minimized.

Show comment
Hide comment
@lautis

lautis Nov 12, 2014

Member

I'd place my bet on eager being faster, too. When you run the performance benchmarks with CPU profiler, ~35% of time is spent in GC. For every event Bacon wraps the value in either Bacon._.always or Bacon._.cached and both create a new function for every value. But it wouldn't be the first time I'm wrong.

Member

lautis commented Nov 12, 2014

I'd place my bet on eager being faster, too. When you run the performance benchmarks with CPU profiler, ~35% of time is spent in GC. For every event Bacon wraps the value in either Bacon._.always or Bacon._.cached and both create a new function for every value. But it wouldn't be the first time I'm wrong.

@rpominov

This comment has been minimized.

Show comment
Hide comment
@rpominov

rpominov Nov 12, 2014

Yes removing laziness might bring quite significant performance boost, btw!

Also take a look at this perf test https://github.com/pozadi/kefir/blob/master/test/perf/perf-specs/deoptimization.coffee

not so random object
----------------------------------------------------------------
Kefir x 2,894,800 ops/sec ±1.94% (66 runs sampled)
Bacon x 613,077 ops/sec ±1.93% (66 runs sampled)
RxJS x 1,479,617 ops/sec ±1.99% (65 runs sampled)
-----------------------
Kefir 1.00   Bacon 0.21   RxJS 0.51


random object
----------------------------------------------------------------
Kefir x 533,397 ops/sec ±6.09% (60 runs sampled)
Bacon x 290,787 ops/sec ±5.29% (61 runs sampled)
RxJS x 451,112 ops/sec ±4.05% (62 runs sampled)
-----------------------
Kefir 1.00   Bacon 0.55   RxJS 0.85

It shows that Kefir works slower when a lot of objects of different type* passing through it, compared to Bacon/Rx. I think the reason that Bacon/Rx suffers less under that condicions is because in those libs values travel wrapped in closures, don't sure thought. But maybe it worth to consider when removing laziness, I'd make a similar perf test at least after removing.

* I mean object structure e.g., {a: 1} and {a: 2} have same type, and {b: 1} has a different one.

Yes removing laziness might bring quite significant performance boost, btw!

Also take a look at this perf test https://github.com/pozadi/kefir/blob/master/test/perf/perf-specs/deoptimization.coffee

not so random object
----------------------------------------------------------------
Kefir x 2,894,800 ops/sec ±1.94% (66 runs sampled)
Bacon x 613,077 ops/sec ±1.93% (66 runs sampled)
RxJS x 1,479,617 ops/sec ±1.99% (65 runs sampled)
-----------------------
Kefir 1.00   Bacon 0.21   RxJS 0.51


random object
----------------------------------------------------------------
Kefir x 533,397 ops/sec ±6.09% (60 runs sampled)
Bacon x 290,787 ops/sec ±5.29% (61 runs sampled)
RxJS x 451,112 ops/sec ±4.05% (62 runs sampled)
-----------------------
Kefir 1.00   Bacon 0.55   RxJS 0.85

It shows that Kefir works slower when a lot of objects of different type* passing through it, compared to Bacon/Rx. I think the reason that Bacon/Rx suffers less under that condicions is because in those libs values travel wrapped in closures, don't sure thought. But maybe it worth to consider when removing laziness, I'd make a similar perf test at least after removing.

* I mean object structure e.g., {a: 1} and {a: 2} have same type, and {b: 1} has a different one.

@skozin

This comment has been minimized.

Show comment
Hide comment
@skozin

skozin Nov 13, 2014

Contributor

I'd prefer removing lazy evaluation. It can be implemented manually when needed, without penalizing the performance in cases when laziness is of no use, which are far more common in UI applications.

Maybe it would make sense to ship these simple helpers mentioned by @phadej with Bacon. Something like this:

Bacon.lazify = (f) -> (x) -> () -> f(x)
_.apply = (f) -> if 'function' is typeof f then f() else f
Observable::unlazify = -> @map _.apply

$out = $in.map(Bacon.lazify(heavywork)).throttle(1000).unlazify()

They won't work in case of combinators with array and object output (combineAsArray, combineTemplate), but that doesn't seem like a huge issue to me.

Contributor

skozin commented Nov 13, 2014

I'd prefer removing lazy evaluation. It can be implemented manually when needed, without penalizing the performance in cases when laziness is of no use, which are far more common in UI applications.

Maybe it would make sense to ship these simple helpers mentioned by @phadej with Bacon. Something like this:

Bacon.lazify = (f) -> (x) -> () -> f(x)
_.apply = (f) -> if 'function' is typeof f then f() else f
Observable::unlazify = -> @map _.apply

$out = $in.map(Bacon.lazify(heavywork)).throttle(1000).unlazify()

They won't work in case of combinators with array and object output (combineAsArray, combineTemplate), but that doesn't seem like a huge issue to me.

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska Nov 13, 2014

Contributor

Thanks guys! Who wants to make a PR?

Contributor

raimohanska commented Nov 13, 2014

Thanks guys! Who wants to make a PR?

@algesten

This comment has been minimized.

Show comment
Hide comment
@algesten

algesten Dec 28, 2014

Contributor

+1 be gone. it cleans up the bacon code as well.

Contributor

algesten commented Dec 28, 2014

+1 be gone. it cleans up the bacon code as well.

@AlexGalays

This comment has been minimized.

Show comment
Hide comment
@AlexGalays

AlexGalays Dec 30, 2014

By the way, are we talking about both kinds of laziness here? ([1] stream not starting until it has subscribers, and [2] lazy evaluation of some mapping functions)

By the way, are we talking about both kinds of laziness here? ([1] stream not starting until it has subscribers, and [2] lazy evaluation of some mapping functions)

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska Dec 30, 2014

Contributor

1 is a cornerstone and will remain
2 is what we're talking about

Contributor

raimohanska commented Dec 30, 2014

1 is a cornerstone and will remain
2 is what we're talking about

@AlexGalays

This comment has been minimized.

Show comment
Hide comment

Perfect!

@philipnilsson

This comment has been minimized.

Show comment
Hide comment
@philipnilsson

philipnilsson Jan 11, 2015

Member

I disagree with removing lazy evaluation on the basis it "might be
confusing". In my opinion the correct way to use Bacon.js is to use it as a
library that provides event streams that are pure values with semantics
specified at the time of constructing them. Code such as
stream.map(getSomethingStateful()) do not follow this principle, and can be
written someStream.sampledBy(stream).

After all we interact w/ steams by subscribing, not by doing .map(effect).
For consistency, we should then provide an analogue to the doAction
combinator, maybe something like stream.pick(function() { $('input').val();
})

I don't think I'll ever agree there are any semantic benefits to removing
lazy eval. If there are performance concerns I'd like to see a significant
difference in a real world application (where strict wins). If this is true
I'm perfectly fine with getting rid of it.

On Wed, Dec 31, 2014 at 12:25 PM, AlexGalays notifications@github.com
wrote:

Perfect!


Reply to this email directly or view it on GitHub
#465 (comment).

Member

philipnilsson commented Jan 11, 2015

I disagree with removing lazy evaluation on the basis it "might be
confusing". In my opinion the correct way to use Bacon.js is to use it as a
library that provides event streams that are pure values with semantics
specified at the time of constructing them. Code such as
stream.map(getSomethingStateful()) do not follow this principle, and can be
written someStream.sampledBy(stream).

After all we interact w/ steams by subscribing, not by doing .map(effect).
For consistency, we should then provide an analogue to the doAction
combinator, maybe something like stream.pick(function() { $('input').val();
})

I don't think I'll ever agree there are any semantic benefits to removing
lazy eval. If there are performance concerns I'd like to see a significant
difference in a real world application (where strict wins). If this is true
I'm perfectly fine with getting rid of it.

On Wed, Dec 31, 2014 at 12:25 PM, AlexGalays notifications@github.com
wrote:

Perfect!


Reply to this email directly or view it on GitHub
#465 (comment).

@raimohanska raimohanska added the 0.8 label Feb 24, 2015

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska Mar 2, 2015

Contributor

I'm still not 100% sure whether is a go or a no-go for 0.8... I'm leaning on the go-side though.

Contributor

raimohanska commented Mar 2, 2015

I'm still not 100% sure whether is a go or a no-go for 0.8... I'm leaning on the go-side though.

@raimohanska raimohanska added 2.0 and removed 0.8 question labels Jan 29, 2018

@raimohanska

This comment has been minimized.

Show comment
Hide comment
@raimohanska

raimohanska Jan 29, 2018

Contributor

Lazy eval will be removed in 2.0.

Contributor

raimohanska commented Jan 29, 2018

Lazy eval will be removed in 2.0.

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