Feliz icon indicating copy to clipboard operation
Feliz copied to clipboard

useElmish ringbuffer

Open alfonsogarciacaro opened this issue 4 years ago • 13 comments

@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?

alfonsogarciacaro avatar Dec 05 '20 06:12 alfonsogarciacaro

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.

Shmew avatar Dec 06 '20 12:12 Shmew

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 avatar Dec 06 '20 13:12 alfonsogarciacaro

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 avatar Dec 17 '20 23:12 alfonsogarciacaro

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.

alfonsogarciacaro avatar Dec 18 '20 22:12 alfonsogarciacaro

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.

Shmew avatar Dec 21 '20 20:12 Shmew

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.

alfonsogarciacaro avatar Dec 22 '20 21:12 alfonsogarciacaro

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.

Shmew avatar Dec 24 '20 10:12 Shmew

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).

alfonsogarciacaro avatar Dec 25 '20 09:12 alfonsogarciacaro

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.

Zaid-Ajaj avatar Dec 25 '20 13:12 Zaid-Ajaj

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.

Shmew avatar Dec 28 '20 18:12 Shmew

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

roboz0r avatar Mar 14 '21 07:03 roboz0r

Is it even used at all? I can't see any calls to Push in the code, only Pull. https://github.com/Zaid-Ajaj/Feliz/blob/52e02e0cd4e5dac48f99134f81ac6d9b3a013234/Feliz.UseElmish/UseElmish.fs#L60-L104

olivercoad avatar Aug 27 '21 09:08 olivercoad

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 😅.

Shmew avatar Sep 04 '21 21:09 Shmew