Synchronize warming the reflection cache (airbnb#244)

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 airbnb#231

Author: gpeal

mvrx/src/main/kotlin/com/airbnb/mvrx/BaseMvRxViewModel.kt

@@ -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
+        get() = stateStore.state
+
     init {
-        // Kotlin reflection has a large overhead the first time you run it
-        // but then is pretty fast on subsequent times. Running these methods now will
-        // initialize kotlin reflect and warm the cache so that when persistState() gets
-        // called synchronously in onSaveInstanceState() on the main thread, it will be
-        // much faster.
-        // This improved performance 10-100x for a state with 100 @PersistState properties.
-        Completable.fromCallable {
-            initialState::class.primaryConstructor?.parameters?.forEach { it.annotations }
-            initialState::class.declaredMemberProperties.asSequence()
-                .filter { it.isAccessible }
-                .forEach { prop ->
-                    @Suppress("UNCHECKED_CAST")
-                    (prop as? KProperty1<S, Any?>)?.get(initialState)
-                }
-        }.subscribeOn(Schedulers.computation()).subscribe()
+        Completable.fromCallable { warmReflectionCache(initialState) }.subscribeOn(Schedulers.computation()).subscribe()
 
         if (this.debugMode) {
             mutableStateChecker = MutableStateChecker(initialState)
@@ -66,8 +55,25 @@ abstract class BaseMvRxViewModel<S : MvRxState>(
         }
     }
 
-    internal val state: S
-        get() = stateStore.state
+    /**
+     * Kotlin reflection has a large overhead the first time you run it
+     * but then is pretty fast on subsequent times. Running these methods now will
+     * initialize kotlin reflect and warm the cache so that when persistState() gets
+     * called synchronously in onSaveInstanceState() on the main thread, it will be much faster.
+     * This improved performance 10-100x for a state with 100 @PersistState properties.
+     *
+     * This is also @Synchronized to prevent a ConcurrentModificationException in kotlin-reflect: https://gist.github.com/gpeal/27a5747b3c351d4bd592a8d2d58f134a
+     */
+    @Synchronized
+    fun warmReflectionCache(initialState: S) {
+        initialState::class.primaryConstructor?.parameters?.forEach { it.annotations }
+        initialState::class.declaredMemberProperties.asSequence()
+            .filter { it.visibility == KVisibility.PUBLIC }
+            .forEach { prop ->
+                @Suppress("UNCHECKED_CAST")
+                (prop as? KProperty1<S, Any?>)?.get(initialState)
+            }
+    }
 
     @CallSuper
     override fun onCleared() {
@@ -108,14 +114,14 @@ abstract class BaseMvRxViewModel<S : MvRxState>(
                     if (changedProp != null) {
                         throw IllegalArgumentException(
                             "Impure reducer set on ${this@BaseMvRxViewModel::class.simpleName}! " +
-                                    "${changedProp.name} changed from ${changedProp.get(firstState)} " +
-                                    "to ${changedProp.get(secondState)}. " +
-                                    "Ensure that your state properties properly implement hashCode."
+                                "${changedProp.name} changed from ${changedProp.get(firstState)} " +
+                                "to ${changedProp.get(secondState)}. " +
+                                "Ensure that your state properties properly implement hashCode."
                         )
                     } else {
                         throw IllegalArgumentException(
                             "Impure reducer set on ${this@BaseMvRxViewModel::class.simpleName}! Differing states were provided by the same reducer." +
-                                    "Ensure that your state properties properly implement hashCode. First state: $firstState -> Second state: $secondState"
+                                "Ensure that your state properties properly implement hashCode. First state: $firstState -> Second state: $secondState"
                         )
                     }
                 }
@@ -562,9 +568,9 @@ abstract class BaseMvRxViewModel<S : MvRxState>(
                     if (activeSubscriptions.contains(deliveryMode.subscriptionId)) {
                         throw IllegalStateException(
                             "Subscribing with a duplicate subscription id: ${deliveryMode.subscriptionId}. " +
-                                    "If you have multiple uniqueOnly subscriptions in a MvRx view that listen to the same properties " +
-                                    "you must use a custom subscription id. If you are using a custom MvRxView, make sure you are using the proper" +
-                                    "lifecycle owner. See BaseMvRxFragment for an example."
+                                "If you have multiple uniqueOnly subscriptions in a MvRx view that listen to the same properties " +
+                                "you must use a custom subscription id. If you are using a custom MvRxView, make sure you are using the proper" +
+                                "lifecycle owner. See BaseMvRxFragment for an example."
                         )
                     }
                     activeSubscriptions.add(deliveryMode.subscriptionId)

mvrx/src/test/kotlin/com/airbnb/mvrx/BaseTest.kt

@@ -37,7 +37,7 @@ abstract class BaseTest {
                 // this prevents StackOverflowErrors when scheduling with a delay
                 override fun scheduleDirect(@NonNull run: Runnable, delay: Long, @NonNull unit: TimeUnit): Disposable = super.scheduleDirect(run, 0, unit)
 
-                override fun createWorker(): Scheduler.Worker = ExecutorScheduler.ExecutorWorker(Executor { it.run() })
+                override fun createWorker(): Worker = ExecutorScheduler.ExecutorWorker(Executor { it.run() }, true)
             }
             RxJavaPlugins.setNewThreadSchedulerHandler { immediate }
             RxJavaPlugins.setComputationSchedulerHandler { immediate }