Do not collapse state change notifications

[hellohuanlin]

I think it's more expected behavior to have 1 reducer to trigger 1 state change. Collapsing notifications can result in missing changes.

For example, if we have value changes from 0 -> 1 -> 0, after collapsing, that state change information is gone.

I agree that collapsing is more performant since it reduces unnecessary UI rendering. But it's possible that developers subscribe to state not just for UI purpose.

Maybe we can allow a customization on whether to collapse or not.

@BenSchwab @elihart @gpeal

[elihart]

If we did this, we could perhaps have whatever observers invalidating UI do some sort of debouncing, or make it easy for subscribe calls in general to add debouncing. I wouldn't want to lose the UI debouncing because it can be important for performance

Unless there is a actual use case where somebody is asking for this I don't see a huge need though

[gpeal]

@elihart MvRx now does postInvalidate() on subscribe rather than invalidate() so maybe we should remove state coalescing. It has come up in a few issues. I'd be fine with it as it is more likely to lead to subtle logging/selectSubscribe bugs to get likely very very small performance improvements.

@BenSchwab @hellohuanlin What do you think?

[elihart]

My main concern with coalescing is UI performance, and Epoxy already coalesces anyway, so it doesn't seem like a huge deal. For the fragment footer case I'd like to see us coalesce those UI updates too, but that shouldn't be hard.

Custom UI layouts could suffer, but maybe we can have an extension or something that makes it easy to debounce state changes

[gpeal]

@elihart we already do because MvRx call postInvalidate() instead of invalidate(). It didn't always do this though.

[hellohuanlin]

I think in fragment level subscribe, we can have a debounce option, for UI components that don't already support debounce out of box.

[hellohuanlin]

Ah I remember there's an issue: with the current design of exposing the state directly out of merge thread (the thread where all reducer/accessor runs), there's gonna be a race condition:

in state store, we expose the state directly:

   // although `subject.value` is thread safe, exposing it can still cause race condition
    internal var state: State
        get() = subject.value

in view model:

    // this is not in merge thread's context, `store.state` is not necessarily the state change 
    // that triggered this invalidation
    notify(store.state)  

let's say we call setState(s1 -> s2) and setState(s2 -> s3), the notified state could be both s3 due to race condition.

[gpeal]

@hellohuanlin what does notify(store.state) do?

[hellohuanlin]

@gpeal sorry the above is just pseudocode on top of my head. after digging into the code, it looks like the synchronous access for the state is only used in withState(viewModel, ...) (StateContainer.kt), and it's only accessible from fragment level mainly for UI purpose, and in view model, only the async version withState { s -> }is accessible. So we should be fine here.

[gpeal]

@hellohuanlin Yup, that is exactly correct. Can we close this issue?

[BenSchwab]

@gpeal @hellohuanlin What's the conclusion here, do we want to emit every state change? I'm okay with that approach with a debounce utility.

[gpeal]

@BenSchwab I'm tempted to remove coalescing because it could break logging now that we have postInvalidate()

@elihart @BenSchwab

[elihart]

That sounds fine to me

[hellohuanlin]

Yeah sounds good to me too. I will close this issue now.

[gpeal]

@hellohuanlin The plan is to implement this :)

[TheLester]

Hi! Any news on "removing coalescing"? In my opinion It would be better to keep it, but have option to disable it

[gpeal]

@TheLester Why would you prefer to keep it?

[TheLester]

@gpeal In case I don't use epoxy, and have frequent state changes, this will affect UI performance

[mradzinski]

@gpeal @elihart I ran through this issue once before, still I think coalescing should exist but be optional (probably enabled by default to keep backwards compat).

Just from the top of my head, but how about having RealMvRxStateStore.set take a second param (nonCoalescing?), same for Jobs.enqueueSetStateBlock, and add an optional param on BaseMvRxViewModel.setState to determine if the new state should coalesce or not? I think that param could be evaluated during RealMvRxStateStore.flushSetStateQueue and either coalesce or not.

[gpeal]

@TheLester @mradzinski Invalidates are posted so that it doesn't occur more than once per frame. Most likely all coalescing would happen within one frame anyway so as long as you are using invalidate(), it is unlikely that the coalescing would save you from many extra invalidates.

I would also encourage you to make your views idempotent so that invalidates aren't expensive.