From 24bce64e0bb5abf51261f4fb27631f903904acf5 Mon Sep 17 00:00:00 2001 From: MarcLeclair Date: Mon, 4 Jan 2021 11:00:34 -0500 Subject: [PATCH] 16373 Count the # of inflations done on startup (#16778) * For #16373: Added performance Inflater to counter # of inflations This class is quite straight forward. The only thing that I have to point out is the onCreateView method. It usually calls its super if you don't override it. The problem with that is that the super.onCreateView actually uses android.view. as a prefix for the XML element it tries to inflate. So if we have an element that isn't part of that package, it'll crash. As I said in the code, a good example is ImageButton. Calling android.view.ImageButton will make the app crash. The method is implemented the same way that PhoneLayoutInflater does (Another example is the AsyncLayoutInflater) * For #16373: Added test for PerformanceInflater This test got quite awkward / complicated fast. I wanted to test the to make sure we don't break *any* of our layouts and to do so, I decided to just retrieve all our XML in our /res/layout folder. However, this gets quite a bit outside of a unit test scope. The point was to get every layouts and get their LayoutID through the resources using the testContext we have. It gets even weirder, since some of the XML tags have special implementation in android. One of them is the tag. That tag actually is inflated by the OS using the Factory2 that the Activity.java implements. In order to get around the fragment issue, we just return a basic FrameLayout since the system LayoutInflater doesn't deal won't ever get a tag to inflate. Another issue was the tag. In order to inflate those, you need 1) a root view and 2) attach your view to it. In order to be able to test those layouts file, I had to create an empty FrameLayout and use it as the root view for testing. Again, I know this is beyond the spirit of a unit test but if we use this inflater, I think it should make sure that no layouts are broken by it. * For #16373: Overrode getSystemService to return PerformanceInflater This allows PerformanceInflater to be called in every inflation to keep track of the number of inflations we do. * For #16373: Added UI test for # of inflations * For #16373: Lint fix * For #167373: Changed the LayoutInflater cloneInContext to take this instead of inflater The inflater parameter is set on the first call from the OS from the Window object. However, the activity itself sets multiple factories on the inflater during its creation (usually through AppCompatDelegateImpl.java). This means that, once we initially set the inflater with a null check, we pass an inflater that has no factory initially. However, since we keep a reference to it, when cloneInContext was called, it cloned the inflater with the original inflater which didn't have any factories set up. This meant that the app would crash on either browserFragment creation or any thing that required appCompat (such as ImageView and ImageButton). Now, passing itself with a cloneInContext means we keep all the factories initially set by the activity or the fragment. * For #16373: Fixed code issues for PR. No behavior change * For #16373: fixed some code nits --- .../perf/StartupExcessiveResourceUseTest.kt | 9 ++ .../java/org/mozilla/fenix/HomeActivity.kt | 14 ++++ .../mozilla/fenix/perf/PerformanceInflater.kt | 76 +++++++++++++++++ .../fenix/perf/PerformanceInflaterTest.kt | 84 +++++++++++++++++++ 4 files changed, 183 insertions(+) create mode 100644 app/src/main/java/org/mozilla/fenix/perf/PerformanceInflater.kt create mode 100644 app/src/test/java/org/mozilla/fenix/perf/PerformanceInflaterTest.kt diff --git a/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt b/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt index 7df918cb1a..3575a7101b 100644 --- a/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt +++ b/app/src/androidTest/java/org/mozilla/fenix/perf/StartupExcessiveResourceUseTest.kt @@ -25,6 +25,7 @@ private const val EXPECTED_RUNBLOCKING_COUNT = 2 private const val EXPECTED_COMPONENT_INIT_COUNT = 42 private const val EXPECTED_VIEW_HIERARCHY_DEPTH = 12 private const val EXPECTED_RECYCLER_VIEW_CONSTRAINT_LAYOUT_CHILDREN = 4 +private const val EXPECTED_NUMBER_OF_INFLATION = 12 private val failureMsgStrictMode = getErrorMessage( shortName = "StrictMode suppression", @@ -54,6 +55,11 @@ private val failureMsgRecyclerViewConstraintLayoutChildren = getErrorMessage( ) + "Please note that we're not sure if this is a useful metric to assert: with your feedback, " + "we'll find out over time if it is or is not." +private val failureMsgNumberOfInflation = getErrorMessage( + shortName = "Number of inflation on start up doesn't match expected count", + implications = "The number of inflation can negatively impact start up time. Having more inflations" + + "will most likely mean we're adding extra work on the UI thread." +) /** * A performance test to limit the number of StrictMode suppressions and number of runBlocking used * on startup. @@ -90,6 +96,8 @@ class StartupExcessiveResourceUseTest { val actualViewHierarchyDepth = countAndLogViewHierarchyDepth(rootView, 1) val actualRecyclerViewConstraintLayoutChildren = countRecyclerViewConstraintLayoutChildren(rootView, null) + val actualNumberOfInflations = InflationCounter.inflationCount.get() + assertEquals(failureMsgStrictMode, EXPECTED_SUPPRESSION_COUNT, actualSuppresionCount) assertEquals(failureMsgRunBlocking, EXPECTED_RUNBLOCKING_COUNT, actualRunBlocking) assertEquals(failureMsgComponentInit, EXPECTED_COMPONENT_INIT_COUNT, actualComponentInitCount) @@ -99,6 +107,7 @@ class StartupExcessiveResourceUseTest { EXPECTED_RECYCLER_VIEW_CONSTRAINT_LAYOUT_CHILDREN, actualRecyclerViewConstraintLayoutChildren ) + assertEquals(failureMsgNumberOfInflation, EXPECTED_NUMBER_OF_INFLATION, actualNumberOfInflations) } } diff --git a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt index 5ca572000e..64cfb77401 100644 --- a/app/src/main/java/org/mozilla/fenix/HomeActivity.kt +++ b/app/src/main/java/org/mozilla/fenix/HomeActivity.kt @@ -14,6 +14,7 @@ import android.os.SystemClock import android.text.format.DateUtils import android.util.AttributeSet import android.view.KeyEvent +import android.view.LayoutInflater import android.view.View import android.view.ViewConfiguration import android.view.WindowManager @@ -89,6 +90,7 @@ import org.mozilla.fenix.library.bookmarks.DesktopFolders import org.mozilla.fenix.library.history.HistoryFragmentDirections import org.mozilla.fenix.library.recentlyclosed.RecentlyClosedFragmentDirections import org.mozilla.fenix.perf.Performance +import org.mozilla.fenix.perf.PerformanceInflater import org.mozilla.fenix.perf.StartupTimeline import org.mozilla.fenix.search.SearchDialogFragmentDirections import org.mozilla.fenix.session.PrivateNotificationService @@ -138,6 +140,8 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { WebExtensionPopupFeature(components.core.store, ::openPopup) } + private var inflater: LayoutInflater? = null + private val navHost by lazy { supportFragmentManager.findFragmentById(R.id.container) as NavHostFragment } @@ -824,6 +828,16 @@ open class HomeActivity : LocaleAwareAppCompatActivity(), NavHostActivity { } } + override fun getSystemService(name: String): Any? { + if (LAYOUT_INFLATER_SERVICE == name) { + if (inflater == null) { + inflater = PerformanceInflater(LayoutInflater.from(baseContext), this) + } + return inflater + } + return super.getSystemService(name) + } + protected open fun createBrowsingModeManager(initialMode: BrowsingMode): BrowsingModeManager { return DefaultBrowsingModeManager(initialMode, components.settings) { newMode -> themeManager.currentTheme = newMode diff --git a/app/src/main/java/org/mozilla/fenix/perf/PerformanceInflater.kt b/app/src/main/java/org/mozilla/fenix/perf/PerformanceInflater.kt new file mode 100644 index 0000000000..e34d7a8d01 --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/perf/PerformanceInflater.kt @@ -0,0 +1,76 @@ +/* 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.content.Context +import android.util.AttributeSet +import android.view.LayoutInflater +import android.view.View +import android.view.ViewGroup +import androidx.annotation.VisibleForTesting +import org.mozilla.fenix.ext.getAndIncrementNoOverflow +import java.lang.reflect.Modifier.PRIVATE +import java.util.concurrent.atomic.AtomicInteger + +private val classPrefixList = arrayOf( + "android.widget.", + "android.webkit.", + "android.app." +) +/** + * Counts the number of inflations fenix does. This class behaves only as an inflation counter since + * it takes the `inflater` that is given by the base system. This is done in order not to change + * the behavior of the app since all we want to do is count the inflations done. + * + */ +open class PerformanceInflater( + inflater: LayoutInflater, + context: Context +) : LayoutInflater( + inflater, + context +) { + + override fun cloneInContext(newContext: Context?): LayoutInflater { + return PerformanceInflater(this, newContext!!) + } + + override fun inflate(resource: Int, root: ViewGroup?, attachToRoot: Boolean): View { + InflationCounter.inflationCount.getAndIncrementNoOverflow() + return super.inflate(resource, root, attachToRoot) + } + + /** + * This code was taken from the PhoneLayoutInflater.java located in the android source code + * (Similarly, AsyncLayoutInflater implements it the exact same way too which can be found in the + * Android Framework). This piece of code was taken from the other inflaters implemented by Android + * since we do not want to change the inflater behavior except to count the number of inflations + * that our app is doing for performance purposes. Looking at the `super.OnCreateView(name, attrs)`, + * it hardcodes the prefix as "android.view." this means that a xml element such as + * ImageButton will crash the app using android.view.ImageButton. This method only works with + * XML tag that contains no prefix. This means that views such as androidx.recyclerview... will not + * work with this method. + */ + @Suppress("EmptyCatchBlock") + @Throws(ClassNotFoundException::class) + override fun onCreateView(name: String?, attrs: AttributeSet?): View? { + for (prefix in classPrefixList) { + try { + val view = createView(name, prefix, attrs) + if (view != null) { + return view + } + } catch (e: ClassNotFoundException) { + // We want the super class to inflate if ever the view can't be inflated here + } + } + return super.onCreateView(name, attrs) + } +} + +@VisibleForTesting(otherwise = PRIVATE) +object InflationCounter { + val inflationCount = AtomicInteger(0) +} diff --git a/app/src/test/java/org/mozilla/fenix/perf/PerformanceInflaterTest.kt b/app/src/test/java/org/mozilla/fenix/perf/PerformanceInflaterTest.kt new file mode 100644 index 0000000000..59698c7831 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/perf/PerformanceInflaterTest.kt @@ -0,0 +1,84 @@ +/* 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.content.Context +import android.util.AttributeSet +import android.view.LayoutInflater +import android.view.View +import android.widget.FrameLayout +import mozilla.components.support.test.robolectric.testContext +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mozilla.fenix.R +import org.mozilla.fenix.helpers.FenixRobolectricTestRunner +import java.io.File + +@RunWith(FenixRobolectricTestRunner::class) +class PerformanceInflaterTest { + + private lateinit var perfInflater: MockInflater + + private val layoutsNotToTest = setOf( + "fragment_browser", + "fragment_add_on_internal_settings" + ) + + @Before + fun setup() { + InflationCounter.inflationCount.set(0) + + perfInflater = MockInflater(LayoutInflater.from(testContext), testContext) + } + + @Test + fun `WHEN we inflate a view,THEN the inflation counter should increase`() { + assertEquals(0, InflationCounter.inflationCount.get()) + perfInflater.inflate(R.layout.fragment_home, null, false) + assertEquals(1, InflationCounter.inflationCount.get()) + } + + @Test + fun `WHEN inflating one of our resource file, the inflater should not crash`() { + val fileList = File("./src/main/res/layout").listFiles() + for (file in fileList!!) { + val layoutName = file.name.split(".")[0] + val layoutId = testContext.resources.getIdentifier( + layoutName, + "layout", + testContext.packageName + ) + + assertNotEquals(-1, layoutId) + if (!layoutsNotToTest.contains(layoutName)) { + perfInflater.inflate(layoutId, FrameLayout(testContext), true) + } + } + } +} + +private class MockInflater( + inflater: LayoutInflater, + context: Context +) : PerformanceInflater( + inflater, + context +) { + + override fun onCreateView(name: String?, attrs: AttributeSet?): View? { + // We skip the fragment layout for the simple reason that it implements + // a whole different inflate which is implemented in the activity.LayoutFactory + // methods. To be able to properly test it here, we would have to copy the whole + // inflater file (or create an activity) and pass our layout through the onCreateView + // method of that activity. + if (name!!.contains("fragment")) { + return FrameLayout(testContext) + } + return super.onCreateView(name, attrs) + } +}