For #24304 - update StartupExcessiveResourceUseTest documentation.

We want this test to be self service for the fenix team so the meaning
of the documentation was updated. Additionally, we clarified why each
heuristic exists to make the test more accessible to self service.
This commit is contained in:
Michael Comella 2022-05-20 17:40:28 -04:00 committed by mergify[bot]
parent ce0c0323a3
commit 4e6e5428ea

View File

@ -20,12 +20,61 @@ import org.mozilla.fenix.R
import org.mozilla.fenix.ext.components
import org.mozilla.fenix.helpers.HomeActivityTestRule
// BEFORE INCREASING THESE VALUES, PLEASE CONSULT WITH THE PERF TEAM.
// BEFORE CHANGING EXPECTED_* VALUES, PLEASE READ THE TEST CLASS KDOC.
/**
* The number of times a StrictMode violation is suppressed during this start up scenario.
* Incrementing the expected value indicates a potential performance regression.
*
* One feature of StrictMode is to detect potential performance regressions and, in particular, to
* detect main thread IO. This includes network requests (which can block for multiple seconds),
* file read/writes (which generally block for tens to hundreds of milliseconds), and file stats
* (like most SharedPreferences accesses, which block for small amounts of time). Main thread IO
* should be replaced with a background operation that posts to the main thread when the IO request
* is complete.
*
* Say no to main thread IO! 🙅
*/
private const val EXPECTED_SUPPRESSION_COUNT = 19
@Suppress("TopLevelPropertyNaming") // it's silly this would have a different naming convention b/c no const
/**
* The number of times we call the `runBlocking` coroutine method on the main thread during this
* start up scenario. Increment the expected values indicates a potential performance regression.
*
* runBlocking indicates that we're blocking the current thread waiting for the result of another
* coroutine. While the main thread is blocked, 1) we can't handle user input and the user may feel
* Firefox is slow and 2) we can't use the main thread to continue initialization that must occur on
* the main thread (like initializing UI), slowing down start up overall. Blocking calls should
* generally be replaced with a slow operation on a background thread launching onto the main thread
* when completed. However, in a very small number of cases, blocking may be impossible to avoid.
*/
private val EXPECTED_RUNBLOCKING_RANGE = 0..1 // CI has +1 counts compared to local runs: increment these together
/**
* The number of `ConstraintLayout`s we inflate that are children of a `RecyclerView` during this
* start up scenario. Incrementing the expected value indicates a potential performance regression.
* THIS IS AN EXPERIMENTAL METRIC and we are not yet confident reducing this count will mitigate
* start up regressions. If you do not find it useful or if it's too noisy, you can consider
* removing it.
*
* ConstraintLayout is expensive to inflate (though fast to measure/layout) so we want to avoid
* creating too many of them synchronously during start up. Generally, these should be inflated
* asynchronously or replaced with cheaper layouts (if they're not too expensive to measure/layout).
* If the view hierarchy uses Jetpack Compose, switching to that is also an option.
*/
private val EXPECTED_RECYCLER_VIEW_CONSTRAINT_LAYOUT_CHILDREN =
4..5 // The messaging framework is not deterministic and could add a +1 to the count
/**
* The number of layouts we inflate during this start up scenario. Incrementing the expected value
* indicates a potential performance regression. THIS IS AN EXPERIMENTAL METRIC and we are not yet
* confident reducing this count will mitigate start up regressions. If you do not find it useful or
* if it's too noisy, you can consider removing it.
*
* Each layout inflation is suspected of having overhead (e.g. accessing each layout resource from
* disk) so suspect inflating more layouts may slow down start up. Ideally, layouts would be merged
* such that there is one inflation that includes all of the views needed on start up.
*/
private val EXPECTED_NUMBER_OF_INFLATION =
14..15 // The messaging framework is not deterministic and could add a +1 to the count
@ -52,19 +101,19 @@ private val failureMsgNumberOfInflation = getErrorMessage(
"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.
* A performance test that attempts to minimize start up performance regressions using heuristics
* rather than benchmarking. These heuristics measure occurrences of known performance anti-patterns
* and fails when the occurrence count changes. If the change indicates a regression, we should
* re-evaluate the PR to see if we can avoid the potential regression and, if not, change the
* expected value. If it indicates an improvement, we can change the expected value. The expected
* values can be updated without consulting the performance team.
*
* This test was written by the perf team.
* See `EXPECTED_*` above for explanations of the heuristics this test currently supports.
*
* StrictMode detects main thread IO, which is often indicative of a performance issue.
* It's easy to suppress StrictMode so we wrote a test to ensure we have a discussion
* if the StrictMode count changes.
*
* RunBlocking is mostly used to return values to a thread from a coroutine. However, if that
* coroutine takes too long, it can lead that thread to block every other operations.
*
* The perf team is code owners for this package so they should be notified when the counts are modified.
* The benefits of a heuristics-based performance test are that it is uses less CI time to get
* results so we can run it more often (e.g. for each PR) and it is less noisy than a benchmark.
* However, the downsides of this style of test is that if a heuristic value increases, it may not
* represent a real, significant performance regression.
*/
class StartupExcessiveResourceUseTest {
@get:Rule