From 9ed43b60b63777836d63cc8d6288f507c33c19fe Mon Sep 17 00:00:00 2001 From: Michael Comella Date: Thu, 20 Feb 2020 15:15:24 -0800 Subject: [PATCH] For #7781: instrument visual completeness for top sites. Eyeballing my output in *Debug builds on my P2, this adds approximately 115ms or slightly less from first frame drawn to visually complete time. --- .../java/org/mozilla/fenix/HomeActivity.kt | 4 +- .../mozilla/fenix/IntentReceiverActivity.kt | 3 + .../viewholders/topsites/TopSitesAdapter.kt | 2 + .../org/mozilla/fenix/perf/Performance.kt | 29 +------- .../fenix/perf/StartupReportFullyDrawn.kt | 69 ++++++++++++++++++ .../org/mozilla/fenix/perf/StartupTimeline.kt | 47 ++++++++++++ .../fenix/perf/StartupTimelineStateMachine.kt | 73 +++++++++++++++++++ .../perf/StartupTimelineStateMachineTest.kt | 44 +++++++++++ 8 files changed, 241 insertions(+), 30 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/perf/StartupReportFullyDrawn.kt create mode 100644 app/src/main/java/org/mozilla/fenix/perf/StartupTimeline.kt create mode 100644 app/src/main/java/org/mozilla/fenix/perf/StartupTimelineStateMachine.kt create mode 100644 app/src/test/java/org/mozilla/fenix/perf/StartupTimelineStateMachineTest.kt diff --git a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt index f6f954a61..119af293a 100644 --- a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt @@ -61,6 +61,7 @@ import org.mozilla.fenix.library.bookmarks.BookmarkFragmentDirections import org.mozilla.fenix.library.history.HistoryFragmentDirections import org.mozilla.fenix.perf.HotStartPerformanceMonitor import org.mozilla.fenix.perf.Performance +import org.mozilla.fenix.perf.StartupTimeline import org.mozilla.fenix.search.SearchFragmentDirections import org.mozilla.fenix.settings.DefaultBrowserSettingsFragmentDirections import org.mozilla.fenix.settings.SettingsFragmentDirections @@ -125,8 +126,6 @@ open class HomeActivity : LocaleAwareAppCompatActivity() { } } - Performance.instrumentColdStartupToHomescreenTime(this) - externalSourceIntentProcessors.any { it.process(intent, navHost.navController, this.intent) } Performance.processIntentIfPerformanceTest(intent, this) @@ -143,6 +142,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity() { supportActionBar?.hide() lifecycle.addObserver(webExtensionPopupFeature) + StartupTimeline.onActivityCreateEndHome(this) } @CallSuper diff --git a/app/src/main/java/org/mozilla/fenix/IntentReceiverActivity.kt b/app/src/main/java/org/mozilla/fenix/IntentReceiverActivity.kt index 61731e2f4..05b917ee1 100644 --- a/app/src/main/java/org/mozilla/fenix/IntentReceiverActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/IntentReceiverActivity.kt @@ -16,6 +16,7 @@ import org.mozilla.fenix.components.getType import org.mozilla.fenix.components.metrics.Event import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.settings +import org.mozilla.fenix.perf.StartupTimeline import org.mozilla.fenix.shortcut.NewTabShortcutIntentProcessor /** @@ -35,6 +36,8 @@ class IntentReceiverActivity : Activity() { intent.stripUnwantedFlags() processIntent(intent) } + + StartupTimeline.onActivityCreateEndIntentReceiver() } suspend fun processIntent(intent: Intent) { diff --git a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSitesAdapter.kt b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSitesAdapter.kt index 7134931a8..d511081d7 100644 --- a/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSitesAdapter.kt +++ b/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/viewholders/topsites/TopSitesAdapter.kt @@ -10,6 +10,7 @@ import androidx.recyclerview.widget.DiffUtil import androidx.recyclerview.widget.ListAdapter import mozilla.components.feature.top.sites.TopSite import org.mozilla.fenix.home.sessioncontrol.TopSiteInteractor +import org.mozilla.fenix.perf.StartupTimeline class TopSitesAdapter( private val interactor: TopSiteInteractor @@ -21,6 +22,7 @@ class TopSitesAdapter( } override fun onBindViewHolder(holder: TopSiteItemViewHolder, position: Int) { + StartupTimeline.onTopSitesItemBound(holder) holder.bind(getItem(position)) } diff --git a/app/src/main/java/org/mozilla/fenix/perf/Performance.kt b/app/src/main/java/org/mozilla/fenix/perf/Performance.kt index a44fd44ce..c9fd37ad0 100644 --- a/app/src/main/java/org/mozilla/fenix/perf/Performance.kt +++ b/app/src/main/java/org/mozilla/fenix/perf/Performance.kt @@ -8,12 +8,9 @@ import android.content.Context import android.content.Intent import android.content.IntentFilter import android.os.BatteryManager -import androidx.core.view.doOnPreDraw -import kotlinx.android.synthetic.main.activity_home.* -import org.mozilla.fenix.HomeActivity import org.mozilla.fenix.onboarding.FenixOnboarding -import android.provider.Settings as AndroidSettings import org.mozilla.fenix.utils.Settings +import android.provider.Settings as AndroidSettings /** * A collection of objects related to app performance. @@ -22,30 +19,6 @@ object Performance { const val TAG = "FenixPerf" private const val EXTRA_IS_PERFORMANCE_TEST = "performancetest" - /** - * Instruments cold startup time for use with our internal measuring system, FNPRMS. This may - * also appear in Google Play Vitals dashboards. - * - * This will need to be rewritten if any parts of the UI are changed to be displayed - * asynchronously. - * - * In the current implementation, we only intend to instrument cold startup to the homescreen. - * To save implementation time, we ignore the fact that the RecyclerView draws twice if the user - * has tabs, collections, etc. open: the "No tabs" placeholder and a tab list. This - * instrumentation will only capture the "No tabs" draw. - */ - fun instrumentColdStartupToHomescreenTime(activity: HomeActivity) { - // For greater accuracy, we could add an onDrawListener instead of a preDrawListener but: - // - single use onDrawListeners are not built-in and it's non-trivial to write one - // - the difference in timing is minimal (< 7ms on Pixel 2) - // - if we compare against another app using a preDrawListener, it should be comparable - // - // Unfortunately, this is tightly coupled to the root view of HomeActivity's view hierarchy - activity.rootContainer.doOnPreDraw { - activity.reportFullyDrawn() - } - } - /** * Processes intent for Performance testing to remove protection pop up ( but keeps the TP * on) and removes the onboarding screen. diff --git a/app/src/main/java/org/mozilla/fenix/perf/StartupReportFullyDrawn.kt b/app/src/main/java/org/mozilla/fenix/perf/StartupReportFullyDrawn.kt new file mode 100644 index 000000000..d08dd59bd --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/perf/StartupReportFullyDrawn.kt @@ -0,0 +1,69 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.perf + +import android.app.Activity +import android.view.View +import androidx.core.view.doOnPreDraw +import kotlinx.android.synthetic.main.activity_home.* +import org.mozilla.fenix.HomeActivity +import org.mozilla.fenix.home.sessioncontrol.viewholders.topsites.TopSiteItemViewHolder +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination.APP_LINK +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination.HOMESCREEN +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupState + +/** + * Instruments the Android framework method [Activity.reportFullyDrawn], which prints time to visual + * completeness to logcat. + * + * At the time of writing (2020-02-26), this functionality is tightly coupled to FNPRMS, our internal + * startup measurement system. However, these values may also appear in the Google Play Vitals + * dashboards. + */ +class StartupReportFullyDrawn { + + // Ideally we'd incorporate this state into the StartupState but we're short on implementation time. + private var isInstrumented = false + + /** + * Instruments "visually complete" cold startup time for app link for use with FNPRMS. + */ + fun onActivityCreateEndHome(state: StartupState, activity: HomeActivity) { + if (!isInstrumented && + state is StartupState.Cold && state.destination == APP_LINK) { + // Instrumenting the first frame drawn should be good enough for app link for now. + isInstrumented = true + attachReportFullyDrawn(activity, activity.rootContainer) + } + } + + /** + * Instruments "visually complete" cold startup time to homescreen for use with FNPRMS. + * + * For FNPRMS, we define "visually complete" to be when top sites is loaded with placeholders; + * the animation to display top sites will occur after this point, as will the asynchronous + * loading of the actual top sites icons. Our focus for visually complete is usability. + * There are no tabs available in our FNPRMS tests so they are ignored for this instrumentation. + */ + fun onTopSitesItemBound(state: StartupState, holder: TopSiteItemViewHolder) { + if (!isInstrumented && + state is StartupState.Cold && state.destination == HOMESCREEN) { + isInstrumented = true + + // Ideally we wouldn't cast to HomeActivity but we want to save implementation time. + val view = holder.itemView + attachReportFullyDrawn(view.context as HomeActivity, view) + } + } + + private fun attachReportFullyDrawn(activity: HomeActivity, view: View) { + // For greater accuracy, we could add an onDrawListener instead of a preDrawListener but: + // - single use onDrawListeners are not built-in and it's non-trivial to write one + // - the difference in timing is minimal (< 7ms on Pixel 2) + // - if we compare against another app using a preDrawListener, as we are with Fennec, it + // should be comparable + view.doOnPreDraw { activity.reportFullyDrawn() } + } +} diff --git a/app/src/main/java/org/mozilla/fenix/perf/StartupTimeline.kt b/app/src/main/java/org/mozilla/fenix/perf/StartupTimeline.kt new file mode 100644 index 000000000..f2561476b --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/perf/StartupTimeline.kt @@ -0,0 +1,47 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.perf + +import androidx.annotation.UiThread +import org.mozilla.fenix.HomeActivity +import org.mozilla.fenix.home.sessioncontrol.viewholders.topsites.TopSiteItemViewHolder +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupActivity +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupState + +/** + * A collection of functionality to instrument, measure, and understand startup performance. The + * responsibilities of this class are to update the internal [StartupState] based on the methods + * called and to delegate calls to its dependencies, which handle other functionality related to + * understanding startup. + * + * This class, and its dependencies, may need to be modified for any changes in startup. + * + * This class is not thread safe and should only be called from the main thread. + */ +@UiThread +object StartupTimeline { + + private var state: StartupState = StartupState.Cold(StartupDestination.UNKNOWN) + private val reportFullyDrawn = StartupReportFullyDrawn() + + fun onActivityCreateEndIntentReceiver() { + advanceState(StartupActivity.INTENT_RECEIVER) + } + + fun onActivityCreateEndHome(activity: HomeActivity) { + advanceState(StartupActivity.HOME) + reportFullyDrawn.onActivityCreateEndHome(state, activity) + } + + fun onTopSitesItemBound(holder: TopSiteItemViewHolder) { + // no advanceState associated with this method. + reportFullyDrawn.onTopSitesItemBound(state, holder) + } + + private fun advanceState(startingActivity: StartupActivity) { + state = StartupTimelineStateMachine.getNextState(state, startingActivity) + } +} diff --git a/app/src/main/java/org/mozilla/fenix/perf/StartupTimelineStateMachine.kt b/app/src/main/java/org/mozilla/fenix/perf/StartupTimelineStateMachine.kt new file mode 100644 index 000000000..806b42768 --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/perf/StartupTimelineStateMachine.kt @@ -0,0 +1,73 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.perf + +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination.APP_LINK +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination.HOMESCREEN +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination.UNKNOWN + +/** + * A state machine representing application startup for use with [StartupTimeline]. Android + * application startup is complex so it's helpful to make all of our expected states explicit, e.g. + * with a state machine, which helps check our assumptions. Unfortunately, because this state machine + * is not used by the framework to determine possible startup scenarios, this is duplicating the + * startup logic and is thus extremely fragile (especially because most devs won't know about this + * class when they change the startup flow!). We may be able to mitigate this with assertions. + * + * To devs changing this class: by design as a state machine, this class should never hold any state + * and should be 100% unit tested to validate assumptions. + */ +object StartupTimelineStateMachine { + + /** + * The states the application passes through during startup. We define these states to help us + * better understand Android startup. Note that these states are not 100% correlated to the + * cold/warm/hot states Google Play Vitals uses. + * + * TODO: link to extensive documentation on cold/warm/hot states when completed. + */ + sealed class StartupState { + /** The state when the application is starting up but is not in memory. */ + data class Cold(val destination: StartupDestination) : StartupState() + } + + /** + * The final screen the user will see during startup. + */ + enum class StartupDestination { + HOMESCREEN, + APP_LINK, + UNKNOWN, + } + + /** + * A list of Activities supported by the app. + */ + enum class StartupActivity { + HOME, + INTENT_RECEIVER, + } + + /** + * Given the current state and any arguments, returns the next state of the state machine. + */ + fun getNextState(currentState: StartupState, startingActivity: StartupActivity): StartupState { + return when (currentState) { + is StartupState.Cold -> nextStateIsCold(currentState, startingActivity) + } + } + + private fun nextStateIsCold(currentState: StartupState.Cold, startingActivity: StartupActivity): StartupState { + return when (currentState.destination) { + UNKNOWN -> when (startingActivity) { + StartupActivity.HOME -> StartupState.Cold(HOMESCREEN) + StartupActivity.INTENT_RECEIVER -> StartupState.Cold(APP_LINK) + } + + // We haven't defined the state machine after these states yet so we return the current state. + else -> currentState + } + } +} diff --git a/app/src/test/java/org/mozilla/fenix/perf/StartupTimelineStateMachineTest.kt b/app/src/test/java/org/mozilla/fenix/perf/StartupTimelineStateMachineTest.kt new file mode 100644 index 000000000..91e658871 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/perf/StartupTimelineStateMachineTest.kt @@ -0,0 +1,44 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.perf + +import org.junit.Assert.assertEquals +import org.junit.Test +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupActivity +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination.APP_LINK +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination.HOMESCREEN +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupDestination.UNKNOWN +import org.mozilla.fenix.perf.StartupTimelineStateMachine.StartupState.Cold +import org.mozilla.fenix.perf.StartupTimelineStateMachine.getNextState + +class StartupTimelineStateMachineTest { + + @Test + fun `GIVEN state cold-unknown WHEN home activity is first shown THEN we are in cold-homescreen state`() { + val actual = getNextState(Cold(UNKNOWN), StartupActivity.HOME) + assertEquals(Cold(HOMESCREEN), actual) + } + + @Test + fun `GIVEN state cold-unknown WHEN intent receiver activity is first shown THEN we are in cold-app-link state`() { + val actual = getNextState(Cold(UNKNOWN), StartupActivity.INTENT_RECEIVER) + assertEquals(Cold(APP_LINK), actual) + } + + @Test + fun `GIVEN state cold + known destination WHEN any activity is passed in THEN we remain in the same state`() { + val knownDestinations = StartupDestination.values().filter { it != UNKNOWN } + val allActivities = StartupActivity.values() + + knownDestinations.forEach { destination -> + val initial = Cold(destination) + allActivities.forEach { activity -> + val actual = getNextState(initial, activity) + assertEquals("$destination $activity", initial, actual) + } + } + } +}