Skip to content
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

Behavior constantly re-evaluating when caching is expected #251

Open
ocharles opened this issue Mar 24, 2022 · 4 comments
Open

Behavior constantly re-evaluating when caching is expected #251

ocharles opened this issue Mar 24, 2022 · 4 comments

Comments

@ocharles
Copy link
Collaborator

This was originally reported at https://old.reddit.com/r/haskell/comments/tk1khp/reactivebanana_controlling_behavior_caching/. I've managed to reproduce the bug in the following:

{-# language BlockArguments #-}

module Main where

import Data.Bool
import Data.Functor
import Debug.Trace
import Reactive.Banana
import Reactive.Banana.Frameworks

main :: IO ()
main = do
  (onSliderChanged, setSlider) <- newAddHandler
  (onChangeCheckbox, changeCheckbox) <- newAddHandler
  (printAH, doPrint) <- newAddHandler

  actuate =<< compile do
    let xs = [True, False, True]

    n <- fromChanges 0 onSliderChanged
    onToggle <- fromAddHandler onChangeCheckbox
    onPrint <- fromAddHandler printAH

    let b :: Behavior String
        b = n <&> \x -> replicate x '.'

    let mk1 vs = trace "sampling 1" . length $ vs
    let mk2 vs = trace "sampling 2" . (* 2) . length $ vs

    let b1 = mk1 <$> b
    let b2 = mk2 <$> b

    b' <- switchB b1 $ bool b1 b2 <$> onToggle

    -- checked <- stepper False onToggle
    -- let b' = liftA3 (\tf x y -> bool x y tf) checked b2 b1

    bChanges <- changes b
    reactimate' $ pure (putStrLn "b changed") <$ bChanges

    b1Changes <- changes b1
    reactimate' $ pure (putStrLn "b1 changed") <$ b1Changes

    b2Changes <- changes b2
    reactimate' $ pure (putStrLn "b2 changed") <$ b2Changes

    b'Changes <- changes b'
    reactimate' $ pure (putStrLn "b' changed") <$ b'Changes

    reactimate $ print <$> b' <@ onPrint

    return ()

  putStrLn "Setting slider"
  setSlider 10
  doPrint ()

  putStrLn "Toggling checkbox"
  changeCheckbox True
  doPrint ()

  putStrLn "Toggling checkbox"
  changeCheckbox False
  doPrint ()

  putStrLn "Toggling checkbox"
  changeCheckbox True
  doPrint ()

  putStrLn "Done"
  return ()

Which prints

Setting slider
b changed
b1 changed
b2 changed
b' changed
sampling 1
10
Toggling checkbox
b' changed
sampling 2
20
Toggling checkbox
b' changed
sampling 1
10
Toggling checkbox
b' changed
sampling 2
20
Done

Note that "sampling 1" or "sampling 2" has happened multiple times despite the underlying Behaviors not changing.

As mentioned in the thread, using liftA3 makes things even worse. If we change b' to defined as:

    checked <- stepper False onToggle
    let b' = liftA3 (\tf x y -> bool x y tf) checked b2 b1

We get

Setting slider
b changed
b1 changed
b2 changed
b' changed
sampling 2
sampling 1
20
Toggling checkbox
b' changed
sampling 2
sampling 1
10
Toggling checkbox
b' changed
sampling 2
sampling 1
20
Toggling checkbox
b' changed
sampling 2
sampling 1
10
Done

which is even more work than before.

@ocharles
Copy link
Collaborator Author

I also note that if I don't have reactimate $ print <$> b' <@ onPrint then I don't get a sampling message at all. This surprises me, because [Note LatchStrictness] states that all latches (and thus I would expect all Behaviors) should be in WHNF at the end of network evaluation - but that doesn't seem to actually be happening.

@ocharles
Copy link
Collaborator Author

ocharles commented Jul 9, 2022

A smaller repro:

{-# language BlockArguments #-}

module Main where

import Data.Functor
import Debug.Trace
import Reactive.Banana
import Reactive.Banana.Frameworks

main :: IO ()
main = do
  (onChangeCheckbox, changeCheckbox) <- newAddHandler

  actuate =<< compile do
    onToggle <- fromAddHandler onChangeCheckbox
    checked  <- stepper False onToggle

    let b1 = traceShowId <$> pure "1"
    let b' = liftA2 (,) checked b1

    reactimate $ return () <$ b' <@ onToggle

  changeCheckbox True
  changeCheckbox False
  changeCheckbox True

Note that changing b1 to be b1 = pure $ traceShowId "1" there is no problem. Also, changing b' to be let b' = traceShowId <$> pure 1 <* checked also doesn't have a problem. It seems to be something specific about having a separate b1 binding that uses fmap.

I also tried let b' = b1 <* checked and this also doesn't have the problem

@ocharles
Copy link
Collaborator Author

ocharles commented Jul 9, 2022

Another weird point: let b' = liftA2 (,) b1 checked does not have a problem, but let b' = liftA2 (,) checked b1 does. So the order here seems to matter. Probably need to get into core at this point.

@mitchellwrosen
Copy link
Collaborator

I made some progress on understanding this one.

Here's a rough diagram of a slightly simplified version of Ollie's example.
Screen Shot 2023-02-26 at 11 10 20 AM

The black text are latches, with nodes in the tree being <*> pointing at the left and right side.

The red arrow shows an onToggle event coming in and affecting the value of the checked latch directly.

We need to calculate the value of the entire latch (the root of the tree), so we will go down depth-first, getting each latch's value, per the definition of applyL and cachedLatch:

applyL :: Latch (a -> b) -> Latch a -> Latch b
applyL lf lx = cachedLatch
({-# SCC applyL #-} getValueL lf <*> getValueL lx)

, _evalL = do
Latch{..} <- liftIO $ Ref.read latch
-- calculate current value (lazy!) with timestamp
(a,time) <- RW.listen eval
liftIO $ if time <= _seenL
then return _valueL -- return old value
else do -- update value
let _seenL = time
let _valueL = a
a `seq` Ref.put latch (Latch {..})
return a

So, say the event occurs at time t=5. When evaluating the whole latch, after getting to the checked child, we'll write t=5 to the evaluation context, since it was directly updated by the event. (The EvalL monad is WriterT Time IO; the time indicates the latest time that this evaluation is recent as of).

Then, later nodes in the tree (such as our traceShowId <$> pure "1", which we expect to be unaffected by the onToggle event and thus not recomputed, enter the same evaluation logic

(a,time) <- RW.listen eval
liftIO $ if time <= _seenL
then return _valueL -- return old value
else do -- update value
let _seenL = time
let _valueL = a
a `seq` Ref.put latch (Latch {..})
return a

only in a context where t=5 is already written to the EvalL context. We'll therefore hit the else-branch which corresponds to recomputing this value rather than using what's cached.

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

No branches or pull requests

2 participants