Bug 1877307 - RFC: Introduce a Store for UI components

[jonalmeida]

Deadline:

Wednesday, February 21th, 2024
Wednesday, March 13th, 2024 (updated)

Pull Request checklist

• Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry or does not need one
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

• Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

1. Click on Checks at the top of the PR page.
2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
4. Click on View task in Taskcluster in the new DETAILS section.
5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1877307

[MozillaNoah]

This sounds pretty cool. Could you add to the RFC a (maybe) hypothetical, follow-up use case of using a non-store dispatcher in practice? In other words, do you have a feature/use-case in mind that would immediately take use of this change (whether it's a refactor of an existing feature or something new in the pipeline)?

Assuming this RFC is accepted, what are your proposed next steps for implementing and consuming this? Are there any technical design docs (or plans to make some)?

[MatthewTighe]

Love this suggestion. My preference would be to allow for Dispatchers directly as parameters to Stores.

In Fenix, our long-term plan is to continue migrating components to lib-state. Does this introduce any risk that these shared thread pools get overwhelmed by work as applications scale? As an alternative, what about sharing CoroutineScopes directly between Stores to take advantage of cooperative computing?

Finally, a couple tangential things to add in case they are worth wrapping into this RFC or follow-up work:

1. It may be helpful to add Store.scope to the MiddlewareContext, so that middleware can inherit test scopes and scopes tied to the lifecycle of the Store.
2. This is more unrelated, but while you're in here thinking about things. The dispatch method defined directly on the MiddlewareContext behaves differently than calling context.store.dispatch. Is it worth renaming (or some other change) in order to make the difference more apparent?

[jonalmeida]

This sounds pretty cool. Could you add to the RFC a (maybe) hypothetical, follow-up use case of using a non-store dispatcher in practice? In other words, do you have a feature/use-case in mind that would immediately take use of this change (whether it's a refactor of an existing feature or something new in the pipeline)?

@MozillaNoah All the stores in Fenix that are meant for specific UI screens (e.g. all the fragment stores) would benefit from having the Store thread be the same as the main thread. Do not change this for the BrowserStore however without performing very careful testing, as it's the at the core of the application.

Assuming this RFC is accepted, what are your proposed next steps for implementing and consuming this? Are there any technical design docs (or plans to make some)?

I am considering writing a blog post about it's usage on mozac.org once this lands. It would be up to your team to decide to make these changes within Fenix.

In Fenix, our long-term plan is to continue migrating components to lib-state. Does this introduce any risk that these shared thread pools get overwhelmed by work as applications scale? As an alternative, what about sharing CoroutineScopes directly between Stores to take advantage of cooperative computing?

@MatthewTighe these are precisely the reasons I would not want to allow any dispatcher to be used. Misuse of which dispatcher to use can lead to very gnarly bugs that will be difficult to debug. We may create a dispatcher that is starved, a dispatcher with shared threads amongst multiple other dispatchers, etc. that would lead to these kinds of bugs that will be a pain to deal with.

The benefits today are from making it easier to have a Store and frontend code running on the same UI thread, so I'd recommend starting with that. If there are benefits to having it on any other dispatcher, we can expand this scope in the future.

Finally, a couple tangential things to add in case they are worth wrapping into this RFC or follow-up work:

1. It may be helpful to add Store.scope to the MiddlewareContext, so that middleware can inherit test scopes and scopes tied to the lifecycle of the Store.

2. This is more unrelated, but while you're in here thinking about things. The dispatch method defined directly on the MiddlewareContext behaves differently than calling context.store.dispatch. Is it worth renaming (or some other change) in order to make the difference more apparent?

Yeah, these are tangential, so please file follow-ups.

[MatthewTighe]

👋 hey Jon! I think this is very close to approval from my perspective, just hoping to read through your response to some of my further comments.

Today our team had a discussion on how we want to handle RFCs moving forward (we want to solve for them recently being a little slow to be finished) and have decided to adopt the "stakeholder" pattern that is likely familiar from other internal technical documents. For now, our team agreement will be that RFCs should have at least 2 internal stakeholders and a 1 external stakeholder if the proposal is an external one. This should make it more clear who is responsible for helping things get over the finish line in a timely manner. In light of that, I have added @boek as the 2nd internal stakeholder and I'm assuming that @csadilek will act as the external.

There will be additional documentation of this decision forthcoming, but in the meantime I've at least added a line to the first RFC to indicate our plans here.

[MatthewTighe]

To close the loop on my comment above about stakeholders:

After further discussion with Jon and the team, we'll hold off on any changes for now in regards to this. Any future plans will be documented through an additional RFC, and we can move forward here using the strategy we have in the pass.

[jonalmeida]

@MatthewTighe after speaking offline with Christian, he provided an alternative solution to the problem of passing in either a dispatcher or config flag:

We don't have any valid use case for having separate dispatchers and our aim is to always run the stores on the UI thread. To make that very clear to the developer, we can create a UiStore that extends the current one that runs with Dispatchers.Main.immediate.

This solves two problems:

1. Making it clear that you are using a UiStore, so expect that you will block the UI thread if you do work on that.
2. The Dispatchers.Main.immediate will ensure we are always processing the actions immediately from when it was called. We do not end up in a state where the action has been put into the queue and not yet processed. It'll also completely remove the need for joinBlocking as well.

I will update the RFC on Monday to reflect this change. Happy weekend! Updated!

[MatthewTighe]

After reading through these latest changes, I'm starting to wonder if perhaps what we are looking for is a way to get immediate results from a Store? I don't think this proposal quite solves that problem because we still rely on Flows which are launched to observe the results of the reducer chain, and may not receive updates immediately based on current thread/coroutine state.

If my understanding here is correct, perhaps we just need a new version of dispatch which is blocking and returns the resultant state? Something like:

fun dispatchBlocking(val action: A) : S

This last part of your comment leads me to believe that might be more what we're looking for:

It'll also completely remove the need for joinBlocking as well.

With the current updates in the proposal, this still wouldn't be true as the signature of dispatch is still:

fun dispatch(action: A): Job

Introducing a blocking dispatch would beg the question of what changes would be needed to protect the reducer chain from long-running operations.

[jonalmeida]

After reading through these latest changes, I'm starting to wonder if perhaps what we are looking for is a way to get immediate results from a Store? I don't think this proposal quite solves that problem because we still rely on Flows which are launched to observe the results of the reducer chain, and may not receive updates immediately based on current thread/coroutine state.

If my understanding here is correct, perhaps we just need a new version of dispatch which is blocking and returns the resultant state? Something like:

fun dispatchBlocking(val action: A) : S

This last part of your comment leads me to believe that might be more what we're looking for:

It'll also completely remove the need for joinBlocking as well.

With the current updates in the proposal, this still wouldn't be true as the signature of dispatch is still:

fun dispatch(action: A): Job

Introducing a blocking dispatch would beg the question of what changes would be needed to protect the reducer chain from long-running operations.

We spoke offline about this review, so summarizing here:

1. Flow observers are always async so that will not change.
2. We do not want more store types that change the dispatchers to other ones because:
   1. We don't have any use cases for it currently.
   2. It increases complexity for developers to understand what is happening if different dispatchers are used.
3. In a UiStore.dispatch the action will be processed immediately. We discussed having a dispatchBlocking but we would need to join on another CoroutineScope which means:
   1. We are changing the dispatch API that returns a Job. Once the dispatch is run, we can already access the store.state immediately after to see the changes.
   2. Also increasing complexity for a new API change. While prototyping this change, we found that it made it more restrictive as we would have to change all dispatchs to dispatchBlocking in current code.
4. TODO: Update the example UiStore to show how we override the dispatcher while being restrictive.

[jonalmeida]

@kycn I noted this bit here as well as it's something that'll help us make incremental wins once this lands and if you're interested in working on this.

[kycn]

Thanks @jonalmeida threads are definitely something on my list! Specifically as in my notes, for better perf and memory consumption, I'll be working on adjusting the threads' priorities according to how crucial they are. This will include the HandlerThreads on Fenix, AC and GV layers.

[jonalmeida]

There are some quick wins in this list of the Fenix/AC threads: https://github.com/mozilla-mobile/fenix/blob/main/docs/List-of-fenix-threads.md

These don't include the Store created threads which we probably have quite a few on startup. Let me know if you want help in working on reducing the Store threads!

[MatthewTighe]

I will just note tangentially that this specific concern would be eliminated if we moved to a full Redux, single-store model, which I would still like to investigate doing (at least in Fenix).

However, this will require quite a bit of prototyping and investigation to make sure we can create ephemeral state that goes out of scope based on the screen so we don't pollute users memory unnecessarily.

Even if we do go that route eventually, I don't foresee using UiStores in the interim from adding an difficulty to that migration.

[jonalmeida]

Yeah, let's talk about a single store model separately. I have not personally seen any proposal that describes what the concrete benefits are.

Even if we do go that route eventually, I don't foresee using UiStores in the interim from adding an difficulty to that migration.

I find this puzzling. Isn't it a bit quick to discount this while we are doing this work to make UI screen stores better? cc: @csadilek if you want to add more clarity that I might have missed in this doc or thread.

[MatthewTighe]

Could we get some additional detail about how dispatch will change for the UiStore? Based on our conversation offline and this comment:

In a UiStore.dispatch the action will be processed immediately. We discussed having a dispatchBlocking but we would need to join on another CoroutineScope which means:
We are changing the dispatch API that returns a Job. Once the dispatch is run, we can already access the store.state immediately after to see the changes.
Also increasing complexity for a new API change. While prototyping this change, we found that it made it more restrictive as we would have to change all dispatchs to dispatchBlocking in current code.

Is the intent to change the underlying Store.dispatch function, or will we somehow override the return type in UiStore to be Unit instead of Job?

Finally - I think I understand pretty well the what here: we want the ability to dispatch an action and immediately examine the updated State in the Store. For example:

uiStore.dispatch(ActionThatUpdatesFlag)
val result = store.state.flag // we have a guarantee that flag has been updated here

However, I think we could still use a little more detail as to why we want this. It seems counter to the typical reactive UI style achieved through observation that is advised these days, even in our own docs. For example, we suggest being reactive and observing the Store.

My assumption is there are corner cases I'm not aware of - like maybe wanting to update the State during a GV delegate callback and immediately inspect the result, but I would still appreciate a clear example here for my own understanding.

[MatthewTighe]

Another alternative to this would be an internal constructor in which we can specify the dispatcher. Personally I would prefer that to an inheritance solution

open class Store<S : State, A : Action> internal constructor(
    initialState: S,
    reducer: Reducer<S, A>,
    middleware: List<Middleware<S, A>> = emptyList(),
    threadNamePrefix: String? = null,
    specifiedDispatcher: CoroutineDispatcher? = null,
) {
    
    constructor(
        initialState: S,
        reducer: Reducer<S, A>,
        middleware: List<Middleware<S, A>> = emptyList(),
        threadNamePrefix: String? = null,
    ) : this(initialState, reducer, middleware, threadNamePrefix, null)
    
    private val threadFactory = StoreThreadFactory(threadNamePrefix)
    private val dispatcher = specifiedDispatcher ?: Executors.newSingleThreadExecutor(threadFactory).asCoroutineDispatcher()
    private val reducerChainBuilder = ReducerChainBuilder(threadFactory, reducer, middleware)
    private val scope = CoroutineScope(dispatcher)

open class UiStore<S : State, A : Action>(
    initialState: S,
    reducer: Reducer<S, A>,
    middleware: List<Middleware<S, A>> = emptyList(),
    threadNamePrefix: String? = null,
) : Store<S, A>(
    initialState,
    reducer,
    middleware,
    threadNamePrefix,
    specifiedDispatcher = Dispatchers.Main.immediate
) {

[jonalmeida]

Thanks for documenting our offline discussion here, I forgot about this!

We didn't want to use this approach because it allows for overriding the dispatcher in the constructor which is the footgun we want to avoid anyone from using and why the RFC was updated not to include that as an option.

[jonalmeida]

Matt clarified offline, and I missed this (sorry!), that the default constructor is internal that has the dispatcher. So yes, this would work.

Both directions solve the same problem, so I have no preference between the styles. I'll update the RFC with this one, thanks!

[MatthewTighe]

I will just note tangentially that this specific concern would be eliminated if we moved to a full Redux, single-store model, which I would still like to investigate doing (at least in Fenix).

However, this will require quite a bit of prototyping and investigation to make sure we can create ephemeral state that goes out of scope based on the screen so we don't pollute users memory unnecessarily.

Even if we do go that route eventually, I don't foresee using UiStores in the interim from adding an difficulty to that migration.

[jonalmeida]

Could we get some additional detail about how dispatch will change for the UiStore? Based on our conversation offline and this comment:

In a UiStore.dispatch the action will be processed immediately. We discussed having a dispatchBlocking but we would need to join on another CoroutineScope which means:
i. We are changing the dispatch API that returns a Job. Once the dispatch is run, we can already access the store.state immediately after to see the changes.
ii. Also increasing complexity for a new API change. While prototyping this change, we found that it made it more restrictive as we would have to change all dispatchs to dispatchBlocking in current code.

Is the intent to change the underlying Store.dispatch function, or will we somehow override the return type in UiStore to be Unit instead of Job?

Sorry, this might be have been poor wording on my part: we don't want to change this API. The Dispatchers.Main.immediate will do all the work for us to ensure the work is completed before the function is done with the dispatch call. We don't have to change the return type or try to do any join call to get this for free.

Finally - I think I understand pretty well the what here: we want the ability to dispatch an action and immediately examine the updated State in the Store. For example:

uiStore.dispatch(ActionThatUpdatesFlag)
val result = store.state.flag // we have a guarantee that flag has been updated here

However, I think we could still use a little more detail as to why we want this. It seems counter to the typical reactive UI style achieved through observation that is advised these days, even in our own docs. For example, we suggest being reactive and observing the Store.

My assumption is there are corner cases I'm not aware of - like maybe wanting to update the State during a GV delegate callback and immediately inspect the result, but I would still appreciate a clear example here for my own understanding.

This is also my bad wording again. In that sample code, the result is meant to indicate that the store.state will have already completed the reducing work by the next line. We are not trying to change the paradigm of observing a store and reacting to it. What we get is a simpler thread model to understand for stores that are typically made for each screen. So you can still observe changes and can reason immediately that a dispatch and an observation below it are happening in that order.

Maybe a better example to help understand this is:

uiStore.dispatch(UpdateState)
uiStore.flowScoped { flow ->
  // We know here that the dispatch would have completed whenever we have an observer here.
}

regularStore.dispatch(UpdateState)
regularStore.flowScoped { flow ->
  // UpdateState may not have been completed by the time we add this observer.
}

In the regularStore case, we could possibly get the initial state and then the update, and we need to then make sure we can handle these cases in our UI code to not have duplicate UI updates (debounces for example). In these UI stores, we don't need to give that complexity to the developers for those simple use cases.

[MatthewTighe]

Ah, the piece I was missing in my understanding is that the stack trace of dispatch isn't suspendable, since the reducer chain is just a series of non-suspending functions. I had assumed that because it was running in a coroutine, any suspension point (like launching in a middleware) could prove problematic, and remove the guarantee you've laid out here:

uiStore.dispatch(UpdateState)
uiStore.flowScoped { flow ->
  // We know here that the dispatch would have completed whenever we have an observer here.
}

I had to figure out a way to test this, and added this code to HomeActivity::onCreate:

    // onCreate
    var count = 0
    for (i in 0..1000) {
        if (logTest()) count ++
    }
    Log.i("tighe", "count: $count")

    // elsewhere
    private fun logTest(): Boolean {
        var x = 0

        CoroutineScope(Main.immediate).launch {
            CoroutineScope(Dispatchers.Default).launch {
                delay(1)
            }
            x = 1
        }

        return x == 1
    }

I'm still not entirely sure what good use cases exist for observing after a call to dispatch (as opposed to just starting the observations along with the lifecycle of the UI), but I don't think we need to keep belaboring that point. I'll add my approval now, given that there are obvious benefits in reducing overall thread count and context switching, and that we can prevent footguns in overriding dispatch through clever visibility modifiers.

Thanks for your patience in helping me understand all the details here!

[MatthewTighe]

One other small implementation detail note while I'm here:

My preference would be for subtypes of UiStore to be typealiases, and leave UiStore as a closed class. This is again because I think inheritance makes code harder to trace and easier to introduce issues. For example, I think this is equivalent to the use cases you're imagining:

class UiStore<S : State, A : Action>(...) : Store<S, A>

typealias FragmentStore = UiStore<FragmentState, FragmentAction>

[rahulsainani]

I think we could remove the dispatcherWithExceptionHandler for UIStore since we're already on the main dispatcher and an exception in the reducer would simply crash the app anyway.

I think removing the logic that creates a thread simply to convert it to a dispatcher makes a lot of sense for UI stores. Fenix can benefit from simply using UIStore for all the screen based stores, A quick search for FragmentStore in Fenix shows up 14 screen based stores. Off the top of my head, I think ReviewQualityCheckStore and TabsTrayStore can use this.

As an iteration, it would benefit to inject CoroutineScope instead of Dispatcher, the same scope can be passed to the middleware. Internally middlewares can use whatever dispatcher making the middleware function main safe.

[jonalmeida]

I think we could remove the dispatcherWithExceptionHandler for UIStore since we're already on the main dispatcher and an exception in the reducer would simply crash the app anyway.

That's a good idea! While it may not seem necessary to make this change because re-throwing on the main thread is fine as-is, this change fits in nicely with the other suggestion @MatthewTighe had added about using an internal constructor to pass in the dependant dispatcher.

I think removing the logic that creates a thread simply to convert it to a dispatcher makes a lot of sense for UI stores. Fenix can benefit from simply using UIStore for all the screen based stores, A quick search for FragmentStore in Fenix shows up 14 screen based stores. Off the top of my head, I think ReviewQualityCheckStore and TabsTrayStore can use this.

💯

While CPU performance is not a driver for this work, I've run our cold startup tests on a release build with a prototype that simply switches the Fenix AppStore to use a UiStore that gives a 6% perf gain. As mentioned in the RFC already, a developer should avoid doing non-UI work on the main thread, so the same rules apply when they use the UiStore.

Pixel 6a, Fenix Release against cc65693

Before:

{'max': 1238.0,
 'mean': 1143.4,
 'median': 1149.5,
 'min': 1073.0,
 'replicate_count': 10,
 'replicates': [1154.0, 1073.0, 1075.0, 1084.0, 1114.0, 1215.0, 1145.0, 1238.0,
                1161.0, 1175.0]}

After:

{'max': 1190.0,
 'mean': 1080.5,
 'median': 1073.0,
 'min': 1017.0,
 'replicate_count': 10,
 'replicates': [1190.0, 1091.0, 1067.0, 1055.0, 1017.0, 1064.0, 1079.0, 1036.0,
                1100.0, 1106.0]}

As an iteration, it would benefit to inject CoroutineScope instead of Dispatcher, the same scope can be passed to the middleware. Internally middlewares can use whatever dispatcher making the middleware function main safe.

Thanks! I've included this bit in the RFC within a new section 'Future work'.

[Mugurell]

Being a bit late to the party - seems like the final plan has been set and approved I want to just ask if there are any recommendations for deciding which stores have to be a UIStore?

[jonalmeida]

Hi @Mugurell ! Surprised the comments are still open here.

Being a bit late to the party - seems like the final plan has been set and approved I want to just ask if there are any recommendations for deciding which stores have to be a UIStore?

The recommendations are to start with 'UI screens' like Rahul hinted above: stores that end with "FragmentStore" are excellent examples of ones that we can switch.

I did try in a prototype to use it on the AppStore too here, but I'd feel better if we validate that it works on smaller individual screens before we move over the AppStore since it's become a very core part of the app now.

EDIT: copying over this discussion to the phabricator implementation review so we can close the loop here and talk about it more in one place.