diff --git a/app/metrics.yaml b/app/metrics.yaml index 58b387e5d..926fff6e4 100644 --- a/app/metrics.yaml +++ b/app/metrics.yaml @@ -1227,23 +1227,6 @@ metrics: - perf-android-fe@mozilla.com - mcomella@mozilla.com expires: "2021-08-11" - activity_state_provider_error: - type: boolean - description: | - The `StartupActivityStateProvider...onActivityStarted` was unexpectedly - called twice. We can use this metric to validate our assumptions about - how these APIs are called. This probe can be removed once we validate - these assumptions. - bugs: - - https://github.com/mozilla-mobile/fenix/issues/18426 - data_reviews: - - https://github.com/mozilla-mobile/fenix/pull/18632#issue-600193452 - data_sensitivity: - - technical - notification_emails: - - perf-android-fe@mozilla.com - - mcomella@mozilla.com - expires: "2021-08-11" preferences: show_search_suggestions: diff --git a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt index c5b520cd5..212b6e92c 100644 --- a/app/src/main/java/org/mozilla/fenix/FenixApplication.kt +++ b/app/src/main/java/org/mozilla/fenix/FenixApplication.kt @@ -193,7 +193,6 @@ open class FenixApplication : LocaleAwareApplication(), Provider { // } components.appStartReasonProvider.registerInAppOnCreate(this) - components.startupActivityStateProvider.registerInAppOnCreate(this) components.startupActivityLog.registerInAppOnCreate(this) initVisualCompletenessQueueAndQueueTasks() diff --git a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt index faaaf6dd2..61f8446cb 100644 --- a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt @@ -275,8 +275,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { components.performance.coldStartupDurationTelemetry.onHomeActivityOnCreate( components.performance.visualCompletenessQueue, - components.appStartReasonProvider, - components.startupActivityStateProvider, + components.startupStateProvider, safeIntent, rootContainer ) diff --git a/app/src/main/java/org/mozilla/fenix/components/Components.kt b/app/src/main/java/org/mozilla/fenix/components/Components.kt index fb8dfda18..4115fc609 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Components.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Components.kt @@ -30,7 +30,6 @@ import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.settings import org.mozilla.fenix.perf.AppStartReasonProvider import org.mozilla.fenix.perf.StartupActivityLog -import org.mozilla.fenix.perf.StartupActivityStateProvider import org.mozilla.fenix.perf.StartupStateProvider import org.mozilla.fenix.perf.lazyMonitored import org.mozilla.fenix.utils.ClipboardHandler @@ -177,7 +176,6 @@ class Components(private val context: Context) { } val appStartReasonProvider by lazyMonitored { AppStartReasonProvider() } - val startupActivityStateProvider by lazyMonitored { StartupActivityStateProvider() } val startupActivityLog by lazyMonitored { StartupActivityLog() } val startupStateProvider by lazyMonitored { StartupStateProvider(startupActivityLog, appStartReasonProvider) } } diff --git a/app/src/main/java/org/mozilla/fenix/perf/ColdStartupDurationTelemetry.kt b/app/src/main/java/org/mozilla/fenix/perf/ColdStartupDurationTelemetry.kt index 0ac29a24b..28d757c26 100644 --- a/app/src/main/java/org/mozilla/fenix/perf/ColdStartupDurationTelemetry.kt +++ b/app/src/main/java/org/mozilla/fenix/perf/ColdStartupDurationTelemetry.kt @@ -11,9 +11,7 @@ import androidx.core.view.doOnPreDraw import mozilla.components.support.base.log.logger.Logger import mozilla.components.support.utils.SafeIntent import org.mozilla.fenix.GleanMetrics.PerfStartup -import org.mozilla.fenix.perf.StartupActivityStateProvider.FirstForegroundActivity -import org.mozilla.fenix.perf.StartupActivityStateProvider.FirstForegroundActivityState -import org.mozilla.fenix.perf.AppStartReasonProvider.StartReason +import org.mozilla.fenix.HomeActivity import java.util.concurrent.TimeUnit private val logger = Logger("ColdStartupDuration") @@ -23,74 +21,35 @@ private val logger = Logger("ColdStartupDuration") * [org.mozilla.fenix.components.metrics.AppStartupTelemetry] class by being simple-to-implement and * simple-to-analyze (i.e. works in GLAM) rather than being a "perfect" and comprehensive measurement. * - * This class relies on external state providers like [AppStartReasonProvider] and - * [StartupActivityStateProvider] that are tricky to implement correctly so take the results with a - * grain of salt. + * This class relies on external state providers like [StartupStateProvider] that are tricky to + * implement correctly so take the results with a grain of salt. */ class ColdStartupDurationTelemetry { fun onHomeActivityOnCreate( visualCompletenessQueue: VisualCompletenessQueue, - startReasonProvider: AppStartReasonProvider, - startupActivityStateProvider: StartupActivityStateProvider, + startupStateProvider: StartupStateProvider, safeIntent: SafeIntent, rootContainer: View ) { - // Optimization: it's expensive to post runnables so we can short-circuit with a subset of the later logic. - if (startupActivityStateProvider.firstForegroundActivityState == - FirstForegroundActivityState.AFTER_FOREGROUND) { - logger.debug("Not measuring: first foreground activity already backgrounded") + // Optimization: it might be expensive to post runnables so we can short-circuit + // with a subset of the later logic. + if (startupStateProvider.shouldShortCircuitColdStart()) { + logger.debug("Not measuring: is not cold start (short-circuit)") return } rootContainer.doOnPreDraw { - // Optimization: we're running code before the first frame so we want to avoid doing anything - // expensive as part of the drawing loop. Recording telemetry took 3-7ms on the Moto G5 (a - // frame is ~16ms) so we defer the expensive work for later by posting a Runnable. - // - // We copy the values because their values may change when passed into the handler. It's - // cheaper to copy the values than copy the objects (= allocation + copy values) so we just - // copy the values even though this copy could happen incorrectly if these values become objects later. - val startReason = startReasonProvider.reason - val firstActivity = startupActivityStateProvider.firstForegroundActivityOfProcess - val firstActivityState = startupActivityStateProvider.firstForegroundActivityState + // This block takes 0ms on a Moto G5: it doesn't seem long enough to optimize. val firstFrameNanos = SystemClock.elapsedRealtimeNanos() - - // On the visual completeness queue, this will report later than posting to the main thread (not - // ideal for pulling out of automated performance tests) but should delay visual completeness less. - visualCompletenessQueue.queue.runIfReadyOrQueue { - if (!isColdStartToThisHomeActivityInstance(startReason, firstActivity, firstActivityState)) { - logger.debug("Not measuring: this activity isn't both the first foregrounded & HomeActivity") - return@runIfReadyOrQueue + if (startupStateProvider.isColdStartForStartedActivity(HomeActivity::class.java)) { + visualCompletenessQueue.queue.runIfReadyOrQueue { + recordColdStartupTelemetry(safeIntent, firstFrameNanos) } - - recordColdStartupTelemetry(safeIntent, firstFrameNanos) } } } - private fun isColdStartToThisHomeActivityInstance( - startReason: StartReason, - firstForegroundActivity: FirstForegroundActivity, - firstForegroundActivityState: FirstForegroundActivityState - ): Boolean { - // This logic is fragile: if an Activity that isn't currently foregrounded is refactored to get - // temporarily foregrounded (e.g. IntentReceiverActivity) or an interstitial Activity is added - // that is temporarily foregrounded, we'll no longer detect HomeActivity as the first foregrounded - // activity and we'll never record telemetry. - // - // Because of this, we may not record values in Beta and Release if MigrationDecisionActivity - // gets foregrounded (I never tested these channels: I think Nightly data is probably good enough for now). - // - // What we'd ideally determine is, "Is the final activity during this start up HomeActivity?" - // However, it's challenging to do so in a robust way so we stick with this simpler solution - // ("Is the first foregrounded activity during this start up HomeActivity?") despite its flaws. - val wasProcessStartedBecauseOfAnActivity = startReason == StartReason.ACTIVITY - val isThisTheFirstForegroundActivity = firstForegroundActivity == FirstForegroundActivity.HOME_ACTIVITY && - firstForegroundActivityState == FirstForegroundActivityState.CURRENTLY_FOREGROUNDED - return wasProcessStartedBecauseOfAnActivity && isThisTheFirstForegroundActivity - } - private fun recordColdStartupTelemetry(safeIntent: SafeIntent, firstFrameNanos: Long) { // This code duplicates the logic for determining how we should handle this intent which // could result in inconsistent results: e.g. the browser might get a VIEW intent but it's diff --git a/app/src/main/java/org/mozilla/fenix/perf/StartupActivityStateProvider.kt b/app/src/main/java/org/mozilla/fenix/perf/StartupActivityStateProvider.kt deleted file mode 100644 index 693281902..000000000 --- a/app/src/main/java/org/mozilla/fenix/perf/StartupActivityStateProvider.kt +++ /dev/null @@ -1,77 +0,0 @@ -/* 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.app.Application -import mozilla.components.support.base.log.logger.Logger -import org.mozilla.fenix.GleanMetrics.Metrics -import org.mozilla.fenix.HomeActivity -import org.mozilla.fenix.android.DefaultActivityLifecycleCallbacks - -private val logger = Logger("StartupActivityState") - -/** - * Provides meta information about the Activities that occur during the initial parts of start up - * and their state. - * - * [registerInAppOnCreate] must be called for this class to work correctly. - */ -class StartupActivityStateProvider { - - enum class FirstForegroundActivity { - TO_BE_DETERMINED, - HOME_ACTIVITY, - UNKNOWN - } - - enum class FirstForegroundActivityState { - BEFORE_FOREGROUND, - CURRENTLY_FOREGROUNDED, - AFTER_FOREGROUND, - } - - /** The first [Activity] that has been foreground in this process lifetime. */ - var firstForegroundActivityOfProcess = FirstForegroundActivity.TO_BE_DETERMINED - private set - - /** The current foreground state of the [firstForegroundActivityOfProcess]. */ - var firstForegroundActivityState = FirstForegroundActivityState.BEFORE_FOREGROUND - private set - - /** - * Registers the handlers needed by this class: this is expected to be called from - * [Application.onCreate]. - */ - fun registerInAppOnCreate(application: Application) { - application.registerActivityLifecycleCallbacks(StateActivityLifecycleCallbacks()) - } - - private inner class StateActivityLifecycleCallbacks : DefaultActivityLifecycleCallbacks { - override fun onActivityStarted(activity: Activity) { - if (firstForegroundActivityOfProcess != FirstForegroundActivity.TO_BE_DETERMINED) { - // This should never happen because we remove the listener in onStop and old activities - // should be stopped before new ones are started but the call order may change slightly - // between devices. - Metrics.activityStateProviderError.set(true) - logger.error("StartupActivityStateProvider...onActivityStarted unexpectedly called twice.") - return - } - - firstForegroundActivityOfProcess = when (activity) { - is HomeActivity -> FirstForegroundActivity.HOME_ACTIVITY - else -> FirstForegroundActivity.UNKNOWN - } - - firstForegroundActivityState = FirstForegroundActivityState.CURRENTLY_FOREGROUNDED - } - - override fun onActivityStopped(activity: Activity) { - firstForegroundActivityState = FirstForegroundActivityState.AFTER_FOREGROUND - - activity.application.unregisterActivityLifecycleCallbacks(this) // no more state updates needed. - } - } -} diff --git a/docs/metrics.md b/docs/metrics.md index 63602a25b..4dff0d9e6 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -313,7 +313,6 @@ In addition to those built-in metrics, the following metrics are added to the pi | engine_tab.kills |[labeled_counter](https://mozilla.github.io/glean/book/user/metrics/labeled_counters.html) |How often was the content process of a foreground (selected) or background tab killed. |[mozilla-mobile/fenix#17864](https://github.com/mozilla-mobile/fenix/pull/17864)|