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

simulate &c. should be Stream a -> Stream b ? #1255

Closed
gergoerdi opened this issue Apr 10, 2020 · 23 comments · Fixed by #1261
Closed

simulate &c. should be Stream a -> Stream b ? #1255

gergoerdi opened this issue Apr 10, 2020 · 23 comments · Fixed by #1261

Comments

@gergoerdi
Copy link
Contributor

gergoerdi commented Apr 10, 2020

The various non-N family of simulate functions are supposed to be applied on infinite lists, and in response return infinite lists. Would it make sense to change their type to consume and return Streams?

I'm asking because I'd like to write something like this:

driveIO :: ([i] -> [o]) -> i -> IO ((o -> IO i) -> IO ())
driveIO f input0 = do
    inChan <- newChan
    writeChan inChan input0

    ins <- getChanContents inChan
    outs <- newMVar $ f ins

    return $ \world -> do
        modifyMVar_ outs $ \(out:outs) -> do
            world out >>= writeChan inChan
            return outs

and this crucially depends on outs never becoming [].

@christiaanb
Copy link
Member

christiaanb commented Apr 10, 2020

I dunno... in principle, simulate is isomorphic to:

:: (Stream a -> Stream b) -> ([a] ->[b])

i.e., why don't you make driveIO :

driveIO :: (Signal dom i -> Signal dom o) -> i -> IO ((o -> IO i) -> IO ())

and use Clash.Signal.Internal ? Even if we were to change the internal representation of Signal down the line, we could always provide a pattern synonym to give the existing behavior.

@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 10, 2020

@christiaanb
Copy link
Member

christiaanb commented Apr 11, 2020

So I agree that Stream would have been the better type to express the semantics of of the non-N variants of simulate, my issue is with changing the API. The non-N variants of simulate will never emit [], and will actually throw an exception if presented with one. So in principle they are safe to use for your purpose: they will never emit a [].

Now, if we really want them to return a Stream a -> Stream b function, I think the way forward would have to be:

module Clash.Prelude where

simulate :: ... -> ([a] -> [b])
module Clash.Prelude_1_4 where

simulate :: ... -> (Stream a -> Stream b)

i.e. if you want to have the new API, you will have to import the new Prelude.

Now... this is just an initial sketch... as I'm unsure how this affect sanely importing code written against Clash.Prelude in code written against Clash.Prelude_1_4.

@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 11, 2020

@christiaanb
Copy link
Member

christiaanb commented Apr 11, 2020

What names would you suggest for these new functions? simulateStream, simulateStreamB, simulateStream_lazy?

@christiaanb
Copy link
Member

christiaanb commented Apr 11, 2020

My issue with new names + deprecation is that you lose the nicer (shorter) names to deprecated functions; or worse, that you change them on the release following the deprecation, e.g.

Clash 1.2

simulate :: ... -> ([a] -> [b])

Clash 1.4

simulate :: ... -> ([a] -> [b]) 
{-# DEPRECATED simulate "will be removed in next release, use simulateStream instead" #-}
simulateStream :: ... -> (Stream a -> Stream b)

Clash 1.6

simulateStream :: ... -> (Stream a -> Stream b)

Clash 1.8

simulate :: ... -> (Stream a -> Stream b)`
simulateStream :: ... -> (Stream a -> Stream) 
{-# DEPRECATED simulateStream "will be removed in next release, use simulate instead" #-}

Clash 1.10

simulate :: ... -> (Stream a -> Stream b)`

i.e. it takes 4 releases to get simulate with the "better" type.

@christiaanb
Copy link
Member

christiaanb commented Apr 11, 2020

I guess a 3rd option would be to export the new Stream-based simulate from a different module, and not from Clash.Prelude. (And perhaps the original/current "batteries-included" Clash.Prelude was a bad idea)

@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 11, 2020

I can't really comment on this issue because I am not familiar with the kind of compatibility guarantees you hope to give.

If it were up to me, I'd just change the type of simulate* in 1.4, and tell to the users "we have a type system so you will know exactly how to change your code"...

@christiaanb
Copy link
Member

christiaanb commented Apr 11, 2020

I want people to be able to use the latest version of Clash, that has all the bug fixes, without them having to change their code. This way, things like blog posts and other "stale" code doesn't go out of date.

@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 12, 2020

Thinking about this a bit more, I am now starting to think that maybe

driveIO :: (Signal dom i -> Signal dom o) -> i -> IO ((o -> IO i) -> IO ())

would in fact be the right idea, because even if simulate was Stream i -> Stream o, there's another property of it that I need to depend on: that it can produce at least one output element by consuming no more than one input element. In other words, that it could be written as an application of a special version of mapAccumL that returns a [NonEmpty o] instead of a [o]. If instead I run simulate internally myself, I can be sure that this property holds for that particular single function.

gergoerdi added a commit to gergoerdi/retroclash-sim that referenced this issue Apr 12, 2020
gergoerdi added a commit to gergoerdi/retroclash-sim that referenced this issue Apr 12, 2020
@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 12, 2020

Just in case anyone is interested, here's the version I'm settling on:

import Clash.Prelude
import Control.Concurrent
import Control.Monad.IO.Class
import Control.Monad (void)

newSink :: IO ([a], a -> IO ())
newSink = do
    chan <- newChan
    list <- getChanContents chan
    let sink = writeChan chan
    return (list, sink)

newSource :: [a] -> IO (IO a)
newSource xs = do
    mvar <- newMVar xs
    return $ modifyMVar mvar $ \(out:outs) -> return (outs, out)

simulateIO
    :: (KnownDomain dom, MonadIO m, NFDataX i, NFDataX o)
    => (HiddenClockResetEnable dom => Signal dom i -> Signal dom o)
    -> i
    -> IO ((o -> m (i, a)) -> m a)
simulateIO circuit input0 = do
    (ins, sinkIn) <- newSink
    sourceOut <- newSource $ simulate circuit ins
    sinkIn input0
    return $ \world -> do
        out <- liftIO sourceOut
        (input, result) <- world out
        liftIO $ sinkIn input
        return result

@martijnbastiaan
Copy link
Member

martijnbastiaan commented Apr 13, 2020

@christiaanb In this particular instance you could introduce a type class instead:

simulate :: StreamLike f => ... -> (f a -> f b)

And put the deprecation warning on the instance for list (if that's even possible). That would reduce the turn-around time to two releases, at the expensive of an ugly type signature in one release.

@martijnbastiaan
Copy link
Member

martijnbastiaan commented Apr 13, 2020

Still, it wouldn't work around the problem of old code snippets.. I guess we could drop Clash.Prelude entirely and ask users to always import Clash.Prelude_1.2 instead. We could even recommend "real" projects to add a Clash.Prelude module to their own hierarchy that just re-exports one of the preludes.

@gergoerdi gergoerdi reopened this Apr 14, 2020
@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 14, 2020

Thinking about this some more...

Even if it returned a Stream i -> Stream o function, it would still take me jumping through multiple hoops to get to driveIO or something similar.

Instead, could you expose a primitive that one could use to simulate into lists, or streams, or IO, or anything else? I am thinking of an interface like the following:

newtype Sim a b = Sim{ stepSim :: a -> (b, Sim a b) }

simulateSteps :: (Signal dom i -> Signal dom o) -> Sim i o

Then, present-day simulate is very easy to recover from simulateSteps; the more tightly typed Stream i -> Stream o interface is also trivial to write; and driveIO can be written without the unsafeInterleaveIO hidden in getChanContents.

What do you think?

@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 14, 2020

I guess the right library type for this would be Automaton with

type Sim = Automaton (->)

stepSim :: Sim i o -> i -> (o, Sim i o)
stepSim (Automaton step) = step

@christiaanb
Copy link
Member

christiaanb commented Apr 14, 2020

We can copy: https://git.smart-cactus.org/ben/clash-testbench/-/blob/master/src/Clash/Testbench/StreamSignal.hs#L17-52 for the implementation of simulateSteps (note that it would then use unsafeInterleaveIO as instead your getChanContents, i.e. there's no way to avoid unsafeInterleaveIO)

@christiaanb
Copy link
Member

christiaanb commented Apr 15, 2020

@gergoerdi I've started the work at: #1261

@christiaanb
Copy link
Member

christiaanb commented Apr 17, 2020

@gergoerdi Are the:

Clash.Explicit.Prelude:

signalAutomaton ::
  forall dom a b.
  (Signal dom a -> Signal dom b) -> 
  Automaton (->) a b

and:

Clash.Prelude:

signalAutomaton ::
  forall dom a b.
  KnownDomain dom =>
  (HiddenClockResetEnable dom => Signal dom a -> Signal dom b) ->
  Automaton (->) a b

functions in #1261 sufficient for your needs?

@christiaanb
Copy link
Member

christiaanb commented Apr 17, 2020

@gergoerdi That is to say: could you test them whether they have space leaks for your use-case?

@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 17, 2020

@gergoerdi That is to say: could you test them whether they have space leaks for your use-case?

As of a478454 I can confirm that my Brainfuck computer doesn't show any signs of space leak with the following implementation of simulateIO_:

 simulateIO_
    :: (KnownDomain dom, MonadIO m, NFDataX i, NFDataX o)
    => (HiddenClockResetEnable dom => Signal dom i -> Signal dom o)
    -> i
    -> IO ((o -> m i) -> m ())
simulateIO_ circuit input0 = do
    let Automaton step = signalAutomaton circuit
    ref <- newMVar $ step input0
    return $ \world -> do
        (out, Automaton step) <- liftIO $ takeMVar ref
        input <- world out
        liftIO $ putMVar ref $ step input

I will later try to also do some performance measurements, but I don't have a setup for that (https://github.com/gergoerdi/clash-bounce-bench doesn't use simulateIO).

Not to look a gift horse too much in the mouth, but I was hoping that with full access to Clash Signal internals, signalAutomaton would be implementable without this much unsafety.

@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 17, 2020

Full source:

{-# LANGUAGE RankNTypes #-}
module RetroClash.Sim.IO
    ( simulateIO
    , simulateIO_
    ) where

import Clash.Prelude
import Control.Concurrent
import Control.Arrow.Transformer.Automaton
import Control.Monad.IO.Class
import Control.Monad (void)

simulateIO
    :: (KnownDomain dom, MonadIO m, NFDataX i, NFDataX o)
    => (HiddenClockResetEnable dom => Signal dom i -> Signal dom o)
    -> i
    -> IO ((o -> m (i, a)) -> m a)
simulateIO circuit input0 = do
    let Automaton step = signalAutomaton circuit
    ref <- newMVar $ step input0
    return $ \world -> do
        (out, Automaton step) <- liftIO $ takeMVar ref
        (input, result) <- world out
        liftIO $ putMVar ref $ step input
        return result

simulateIO_
    :: (KnownDomain dom, MonadIO m, NFDataX i, NFDataX o)
    => (HiddenClockResetEnable dom => Signal dom i -> Signal dom o)
    -> i
    -> IO ((o -> m i) -> m ())
simulateIO_ circuit input0 = do
    sim <- simulateIO circuit input0
    return $ \world -> do
        void $ sim $ \output -> do
            input <- world output
            return (input, ())

@christiaanb
Copy link
Member

christiaanb commented Apr 17, 2020

I have no idea how to implement signalAutomaton without the unsafety. I mean.. you have to apply the function: Stream a -> Stream b to an entire Stream a, to get even a single element b; but excepts for the current sample a, the rest of elements of the Stream a is "out-of-scope" because those elements are lamba-bound by the subsequent Automatons.

@gergoerdi
Copy link
Contributor Author

gergoerdi commented Apr 18, 2020

I have no idea how to implement signalAutomaton without the unsafety. I mean.. you have to apply the function: Stream a -> Stream b to an entire Stream a, to get even a single element b; but excepts for the current sample a, the rest of elements of the Stream a is "out-of-scope" because those elements are lamba-bound by the subsequent Automatons.

Well my comment was based on the idea that unlike a library like what I'm writing, you can do any needed violence to Signal internals as long as the interface remains.

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

Successfully merging a pull request may close this issue.

3 participants