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

useElmish ringbuffer #288

Closed
alfonsogarciacaro opened this issue Dec 5, 2020 · 13 comments
Closed

useElmish ringbuffer #288

alfonsogarciacaro opened this issue Dec 5, 2020 · 13 comments

Comments

@alfonsogarciacaro
Copy link
Contributor

@Shmew Not sure if it's a silly thing, but I was thinking the other day if it would make sense to use a singleton for the ringbuffer that managed the messages of all components (the messages would be boxed and contain a reference to the target). This would have the benefit of not allocating an individual ring buffer per component and it may also make it easier to log all the messages from the app. What do you think?

@Shmew
Copy link
Collaborator

Shmew commented Dec 6, 2020

Yeah that would be nice. The only issue is the entire thing needs to stay inside the React tree in order to be compatible with concurrent mode. This can be solved by using something like context as way of fetching the dependency, but it sounds like it may be a little tough in addition to requiring the user to provide a context provider and/or some other configuration.

I'm also not sure how the plumbing would work with that, as we'd have to handle where the effects are actually consumed. Would the first component just do them all? I could see it having an impact on performance.

@alfonsogarciacaro
Copy link
Contributor Author

Ah, I see. Didn't think of the concurrent mod e, thanks a lot for the explanation! Ok, will close for now and maybe we can revisit later.

@alfonsogarciacaro
Copy link
Contributor Author

Sorry @Shmew, I keep thinking about this 😅 If I'm not mistaken (though I might very well be) the ring buffer is used in Elmish because it needs to be compatible with platforms where the UI and the update function may run in different threads. But useElmish will only run in JS, so why not just do something like this?

@alfonsogarciacaro
Copy link
Contributor Author

Maybe I can answer myself: is the ring buffer used to make sure the messages returned by the commands are delivered in the same order the commands were created? This makes sense although it can still be surprising for a user if they launch 2 http requests, the first gets blocked for some reason and this prevents the response from the second one.

@Shmew
Copy link
Collaborator

Shmew commented Dec 21, 2020

I hadn't ever given it much thought since that's how normal Fable elmish is run.

Maybe I can answer myself: is the ring buffer used to make sure the messages returned by the commands are delivered in the same order the commands were created? This makes sense although it can still be surprising for a user if they launch 2 http requests, the first gets blocked for some reason and this prevents the response from the second one.

It never awaits on any promises, so there shouldn't be any blocking. It might be able to function fine just using a normal collection.

@alfonsogarciacaro
Copy link
Contributor Author

Thanks for checking @Shmew! Sorry I'm getting a bit obsessed with the ring buffer. It's just I remember all the discussions we had about asynchronous updates in Elmish and it concerns me a bit that we have a piece of code we don't really know what it's doing. IIRC the ring buffer was a replacement of MailboxProcessor precisely because it wasn't really asynchronous in Fable.

Users are also always confused about whether update runs synchronously or not. My current thinking is that update should be synchronous and fast and asynchronous/slow actions should be left to the commands. The difference shouldn't be too important in JS though, given that you cannot spawn new threads. It should be enough to wrap command calls in a Promise or setTimeout(..., 0) to make sure the execution will happen in the next JS event loop. Although in most cases commands will already spawn promises by themselves, and if you're planning to block the thread running a factorial on a big number, using a command won't help and it won't make a big difference either whether the UI gets blocked in this or the next event tick.

@Shmew
Copy link
Collaborator

Shmew commented Dec 24, 2020

Currently all of the processing is done as a promise (inside an effect hook).

Yeah it's worth doing some profiling to see if the data structure is slow and if it's even needed. The thing is it is nice since we do want a FIFO rather than a stack. I did do profiling when I was writing all of this and it was pretty quick as far as I could tell. It's a shame FSSF Slack doesn't have history as I sent pictures to @Zaid-Ajaj (and talked his ears off) about it.

@alfonsogarciacaro
Copy link
Contributor Author

Thanks @Shmew! I was not so concerned about the performance (though that's very important too) but about the complexity to understand what exactly useElmish does. Basically my question would be, if the commands are just functions accepting a dispatch function, why do we need a structure at all instead of just firing those functions immediately (or wrapped in a promise or a setTimeout).

@Zaid-Ajaj Zaid-Ajaj reopened this Dec 25, 2020
@Zaid-Ajaj
Copy link
Owner

Let's reopen the issue and see how we can simplify the structure used, if a ring buffer isn't needed then we can remove. I thought this was the way to simulate a Program.withReactSynchronous but I am not sure anymore. At least, I think the implementation should be like that anyways without input element weirdness etc.

@alfonsogarciacaro alfonsogarciacaro changed the title UseElmish, singleton ringbuffer? useElmish ringbuffer Dec 26, 2020
@Shmew
Copy link
Collaborator

Shmew commented Dec 28, 2020

Basically my question would be, if the commands are just functions accepting a dispatch function, why do we need a structure at all instead of just firing those functions immediately (or wrapped in a promise or a setTimeout).

So we do fire off all new commands immediately, the main thing is that we want to ensure the message queue is FIFO. We could do this using a list by creating a new list and appending it, or reversing the current list and then adding it to the head followed by another reverse. Both of those are just going to be doing what this data structure does though. I think this would be slower as well, but obviously we'd need to profile it.

I thought this was the way to simulate a Program.withReactSynchronous but I am not sure anymore.

This behaves that way regardless of the internal data structure. This is the case because we're using hooks and updating state within an effect hook.

@roboz0r
Copy link
Contributor

roboz0r commented Mar 14, 2021

Here's a reasonably efficient immutable FIFO implementation with Lists. Reverse is only called when the outgoing list is empty and the incoming has built up items so execution is typically O(1). I had a look at the existing useElmish implementation but didn't really understand what was going on so wasn't that tempted to change it.

type ListQueue<'T> private (incoming:'T list, outgoing:'T list) =
    new () = ListQueue([], [])

    member this.Push item = 
        match incoming, outgoing with
        | [], [] -> ListQueue([], [item])
        | _ -> ListQueue(item :: incoming, outgoing)

    member this.Pop () = 
        match outgoing with
        | item :: outgoing -> 
            ListQueue(incoming, outgoing), item
        | [] -> 
            match List.rev incoming with
            | [] -> failwith "Cannot Pop from an empty ListQueue"
            | item :: outgoing -> ListQueue([], outgoing), item

    member this.IsEmpty = 
        match incoming, outgoing with
        | [], [] -> true
        | _ -> false

@olivercoad
Copy link
Contributor

Is it even used at all? I can't see any calls to Push in the code, only Pull.

static member useElmish<'State,'Msg> (init: 'State * Cmd<'Msg>, update: 'Msg -> 'State -> 'State * Cmd<'Msg>, dependencies: obj[]) =
let state = React.useRef(fst init)
let ring = React.useRef(RingBuffer(10))
let childState, setChildState = React.useState(fst init)
let token = React.useCancellationToken()
let setChildState () =
JS.setTimeout(fun () ->
if not token.current.IsCancellationRequested then
setChildState state.current
) 0 |> ignore
let rec dispatch (msg: 'Msg) =
promise {
let mutable nextMsg = Some msg
while nextMsg.IsSome && not (token.current.IsCancellationRequested) do
let msg = nextMsg.Value
let (state', cmd') = update msg state.current
cmd' |> List.iter (fun sub -> sub dispatch)
nextMsg <- ring.current.Pop()
state.current <- state'
setChildState()
}
|> Promise.start
let dispatch = React.useCallbackRef(dispatch)
React.useEffect((fun () ->
React.createDisposable(fun () ->
getDisposable state.current
|> Option.iter (fun o -> o.Dispose())
)
), dependencies)
React.useEffect((fun () ->
state.current <- fst init
setChildState()
snd init
|> List.iter (fun sub -> sub dispatch)
), dependencies)
React.useEffect(fun () -> ring.current.Pop() |> Option.iter dispatch)
(childState, dispatch)

@Shmew
Copy link
Collaborator

Shmew commented Sep 4, 2021

Is it even used at all? I can't see any calls to Push in the code, only Pull.

That... actually seems to be the case 😅.

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

No branches or pull requests

5 participants