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

MvRxStateStore based on kotlin coroutines #79

Closed
ATizik opened this issue Sep 3, 2018 · 10 comments · Fixed by #347
Closed

MvRxStateStore based on kotlin coroutines #79

ATizik opened this issue Sep 3, 2018 · 10 comments · Fixed by #347

Comments

@ATizik
Copy link

ATizik commented Sep 3, 2018

Since MvRxStateStore can be a performance bottleneck on screens with state changing very frequently, maybe it would be beneficial to implement it with coroutines instead of Rx?
I can provide performance comparisons to determine actual performance gain in case it's needed

@gpeal
Copy link
Collaborator

gpeal commented Sep 3, 2018

@ATizik Do you have a MvRxStateStore implementation with coroutines? The main reason we used rx is our own familiarity with rx but if coroutines perform better, it would be worth investigating for sure.

cc @BenSchwab @elihart @hellohuanlin

@ATizik
Copy link
Author

ATizik commented Sep 4, 2018

@gpeal
I do have something similar in our projects, since you're interested I'll adapt it to MvRx api and share with you

@gpeal
Copy link
Collaborator

gpeal commented Sep 5, 2018

@ATizik Would love to see a PR. If you have a good way to benchmark it, that would be great too!

@ATizik
Copy link
Author

ATizik commented Sep 7, 2018

@gpeal
Here it is #84
Speed difference was negligible on existing test, maybe I need a heavier state and operations, i'm in the process of writing more comprehensive benchmark. I'll also test a memory consumption later.

@luozejiaqun
Copy link

I still do not understand Why can't simply use the Main Thread to avoid race conditions of setting the state? The stateReducer is just updating the state, why can't put that in the main thread. And if the state changes very frequently, I would prefer to use the LiveData to avoid creating new states.

@ATizik
Copy link
Author

ATizik commented Nov 22, 2018

@luozejiaqun
It's better to offload it to the background, there is nothing specific about main thread that's better for avoiding race conditions than any other thread.
I'm not sure how LiveData will help avoid creating new states, we still have to run stateReducer the same amount of times

@luozejiaqun
Copy link

First question, if there is nothing specific about main thread, why don't use the main thread. Since every ViewModel owns a StateStore, using main thread will avoid creating other threads. I'm not familiar with kotlin coroutines, but the fundamental thing is still threads, less threads achieve the same goal, this my rough understanding. If the stateReducer is simple, there is no reason why can't put it in main thread.
Second question, there is not contradiction between State and LiveData, why can't put a property in LiveData and remove it from the State. In this way, the property can be mutable, changes it and sets it to LiveData's value, then done. The fact is there are some properties we can't use in State, such as SparseArray and LongSparseArray. I put these properties in LiveData and that works well for me.

@gpeal
Copy link
Collaborator

gpeal commented Nov 27, 2018

@luozejiaqun A fundamental principle of MvRx is that you observe a stream of immutable state. Storing a SparseArray in LiveData actually enables you to write buggy code. If you modify an element in the array, your LiveData won't update. That's why MvRx enforces immutability.

Regarding the background thread, there are performance advantages in keeping it off the main thread but we have implemented it in such a way that there should be no cognitive overhead for the developer. Why not offload things off the main thread if we can?

@luozejiaqun
Copy link

@gpeal I know MvRx enforces immutability, because of the reason, I can't use SparseArray in the State. The MvRx's suggestion is using immutable list, but I don't think that makes sense. Because of performance advantages we use SparseArray, immutable list can't meet that requirements. LiveData maybe buggy, but that's my only choice for now.

Regarding the background thread, I know the performance advantages.But using the main thread and no more new threads also has performance advantages.

@gpeal
Copy link
Collaborator

gpeal commented Jan 8, 2019

@luozejiaqun Let's separate that discussion from the coroutines state store

gpeal added a commit that referenced this issue May 14, 2020
This PR converts the internal implementation from RxJava to Coroutines. This was not designed to be a breaking change so no external APIs still use Disposable.

However, a new API was added: ViewModel.stateFlow This property allows consumers to easily combine MvRx state with other operations or chain it to other Flow operators. Thoughts on this API? It doesn't violate the primary concern of providing synchronous state access from within a ViewModel.

Once this is landed, I think it is time to create the mvrx-rxjava2 artifact and start splitting off RxJava for good. Thoughts?

Fixes #296
Fixes #79
#130
elihart pushed a commit that referenced this issue May 25, 2020
This PR converts the internal implementation from RxJava to Coroutines. This was not designed to be a breaking change so no external APIs still use Disposable.

However, a new API was added: ViewModel.stateFlow This property allows consumers to easily combine MvRx state with other operations or chain it to other Flow operators. Thoughts on this API? It doesn't violate the primary concern of providing synchronous state access from within a ViewModel.

Once this is landed, I think it is time to create the mvrx-rxjava2 artifact and start splitting off RxJava for good. Thoughts?

Fixes #296
Fixes #79
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