baconjs / bacon.js

Functional reactive programming library for TypeScript and JavaScript
https://baconjs.github.io
MIT License
6.47k stars 331 forks source link

Lazy evaluation: good or bad? #465

Closed raimohanska closed 6 years ago

raimohanska commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

Thanks guys! Who wants to make a PR?

algesten commented 9 years ago

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

AlexGalays commented 9 years ago

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 commented 9 years ago

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

AlexGalays commented 9 years ago

Perfect!

philipnilsson commented 9 years ago

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 https://github.com/baconjs/bacon.js/issues/465#issuecomment-68436766.

raimohanska commented 9 years ago

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 commented 6 years ago

Lazy eval will be removed in 2.0.