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

Synchronize warming the reflection cache #244

Merged
merged 2 commits into from Jun 17, 2019
Merged

Conversation

gpeal
Copy link
Collaborator

@gpeal gpeal commented Jun 14, 2019

In addition, we should have been checking for KVisibility == PUBLIC not isAccessible. As is, the second part of the reflection warming was filtering out all properties.

Here is a benchmark in which I switched from FlowViewModel in which I added 80 @PersistState properties to another app and back several times with background processes turned off
without declaredMemberProperties warming:

06-14 08:05:05.240  5295  5344 D Gabe    : persistState: 21
06-14 08:05:05.308  5295  5295 D Gabe    : persistState: 7
06-14 08:05:09.341  5390  5431 D Gabe    : persistState: 32
06-14 08:05:12.328  5390  5390 D Gabe    : persistState: 7
06-14 08:05:16.948  5484  5517 D Gabe    : persistState: 28
06-14 08:05:19.604  5484  5484 D Gabe    : persistState: 4
06-14 08:05:24.098  5569  5599 D Gabe    : persistState: 37
06-14 08:05:24.568  5569  5569 D Gabe    : persistState: 5
06-14 08:05:28.866  5652  5682 D Gabe    : persistState: 16
06-14 08:05:29.542  5652  5652 D Gabe    : persistState: 5
06-14 08:05:34.043  5736  5769 D Gabe    : persistState: 21

with declaredMemberProperties warming:

06-14 08:05:50.617  5865  5916 D Gabe    : persistState: 5
06-14 08:05:50.670  5865  5865 D Gabe    : persistState: 8
06-14 08:05:54.826  5971  6004 D Gabe    : persistState: 9
06-14 08:05:54.981  5971  5971 D Gabe    : persistState: 3
06-14 08:05:59.318  6067  6100 D Gabe    : persistState: 5
06-14 08:05:59.811  6067  6067 D Gabe    : persistState: 5
06-14 08:06:03.791  6149  6185 D Gabe    : persistState: 9
06-14 08:06:04.899  6149  6149 D Gabe    : persistState: 4
06-14 08:06:10.128  6235  6265 D Gabe    : persistState: 5
06-14 08:06:15.794  6318  6351 D Gabe    : persistState: 6

Fixes #231

@gpeal gpeal requested review from BenSchwab and elihart June 14, 2019 15:08
@@ -41,22 +41,11 @@ abstract class BaseMvRxViewModel<S : MvRxState>(
private val lastDeliveredStates = ConcurrentHashMap<String, Any>()
private val activeSubscriptions = Collections.newSetFromMap(ConcurrentHashMap<String, Boolean>())

internal val state: S
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this prop above init for style

Copy link
Contributor

@elihart elihart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, seems good

@gpeal gpeal merged commit 4bf5e11 into master Jun 17, 2019
@gpeal gpeal deleted the gpeal/synchronize-warm-cache branch June 17, 2019 04:05
@tasomaniac
Copy link
Contributor

Were you be able to check if this fixes the crash mentioned in #231? Our app starts to crash when R8/Proguard is enabled and I suspect it is because of this change.

I wonder if it is possible to make this change configurable? Users would enable it if they have reflection based performance issues with their usage.

@gpeal
Copy link
Collaborator Author

gpeal commented Jul 29, 2019

@tasomaniac at Tonal, we're on 1.0.2 and we haven't seen the crash since so I'm going to say that it probably does but I can't say 100% since it's not easily reproducible.

I think every app will have reflection based performance issues since kotlin-reflect init is slow. There is probably a way to fix the R8 issue while leaving this intact. Could you try and get the mvrx sample app to crash by adding R8?

@rossbacher @BenSchwab Any chance you can help out with this as well? It'll block Airbnb from upgrading.

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 this pull request may close these issues.

Kotlin Reflect ConcurrentModificationException
4 participants