15278 detekt rule runblocking (#15942)
* For #15278: added CoroutineManager to count runBlocking calls * For #15278: Added actual detekt rule for runblocking and its config to the yaml * For #15278: Added unit test for RunblockingCounter * For #15278: renamed StrictModeStartupSuppressionCountTest.kt to PerformanceStartupTest.kt and added runBlockingCount test * Lint fix * For #15278: made runblocking a Long to prevent overflow * For #15278: fixed MozRunblocking name, description and moved RunBlockingCounter to perf package * For #15278:Renamed MozillaRunblockingCheck to MozillaRunBlockingCheck * For #15278: Added setup for unit test, since it failed without restting counter * For #15278: Fixed naming for RunBlocking lint check * For #15278: removed changes made to test to use runBlockingIncrement * For #15728: added test exclusion for runBlocking check * For #15278: changed null check and added Synchronized to count setter * For #15278: fix for nits * For #15278: added StartupExcessiveResourceUseTest to CODEOWNERS * For #15278: fixed for nits * For #15278: Moved increment function to extension function and fixed indentation * For #15278: Added tests for Atomic Integer extension and nit fixupstream-sync
MarcLeclair
4 years ago
committed by
GitHub
parent
d46fc7b142
commit
7b1af41b40
@ -0,0 +1,19 @@
|
||||
/* 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.ext
|
||||
|
||||
import java.util.concurrent.atomic.AtomicInteger
|
||||
|
||||
/**
|
||||
* Increases an AtomicInteger safely.
|
||||
*/
|
||||
fun AtomicInteger.getAndIncrementNoOverflow() {
|
||||
var prev: Int
|
||||
var next: Int
|
||||
do {
|
||||
prev = this.get()
|
||||
next = if (prev == Integer.MAX_VALUE) prev else prev + 1
|
||||
} while (!this.compareAndSet(prev, next))
|
||||
}
|
||||
@ -0,0 +1,45 @@
|
||||
/* 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/. */
|
||||
|
||||
// This class implements the alternative ways to invoke runBlocking with some
|
||||
// monitoring by wrapping the raw methods. This lint check tells us not to use the raw
|
||||
// methods so we suppress the check.
|
||||
@file:Suppress("MozillaRunBlockingCheck")
|
||||
|
||||
package org.mozilla.fenix.perf
|
||||
|
||||
import androidx.annotation.VisibleForTesting
|
||||
import kotlinx.coroutines.CoroutineScope
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import org.mozilla.fenix.ext.getAndIncrementNoOverflow
|
||||
import java.util.concurrent.atomic.AtomicInteger
|
||||
import kotlin.coroutines.CoroutineContext
|
||||
|
||||
/**
|
||||
* Counts the number of runBlocking calls made
|
||||
*/
|
||||
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
|
||||
object RunBlockingCounter {
|
||||
val count = AtomicInteger(0)
|
||||
}
|
||||
|
||||
/**
|
||||
* Wrapper around `runBlocking`. RunBlocking seems to be a "fix-all" to return values to the thread
|
||||
* where the coroutine is called. The official doc explains runBlocking: "Runs a new coroutine and
|
||||
* blocks the current thread interruptibly until its completion`. This can block our main thread
|
||||
* which could lead to significant jank. This wrapper aims to count the number of runBlocking call
|
||||
* to try to limit them as much as possible to encourage alternatives solutions whenever this function
|
||||
* might be needed.
|
||||
*/
|
||||
fun <T> runBlockingIncrement(
|
||||
context: CoroutineContext? = null,
|
||||
action: suspend CoroutineScope.() -> T
|
||||
): T {
|
||||
RunBlockingCounter.count.getAndIncrementNoOverflow()
|
||||
return if (context != null) {
|
||||
runBlocking(context) { action() }
|
||||
} else {
|
||||
runBlocking { action() }
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,40 @@
|
||||
/* 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.ext
|
||||
|
||||
import org.junit.Assert.assertEquals
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.launch
|
||||
import kotlinx.coroutines.runBlocking
|
||||
import org.junit.Test
|
||||
import java.util.concurrent.atomic.AtomicInteger
|
||||
|
||||
/* 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/. */
|
||||
|
||||
class AtomicIntegerTest {
|
||||
|
||||
@Test
|
||||
fun `Safely increment an AtomicInteger from different coroutines`() {
|
||||
val integer = AtomicInteger(0)
|
||||
runBlocking {
|
||||
for (i in 1..2) {
|
||||
launch(Dispatchers.Default) {
|
||||
integer.getAndIncrementNoOverflow()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
assertEquals(integer.get(), 2)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `Incrementing the AtomicInteger should not overflow`() {
|
||||
val integer = AtomicInteger(Integer.MAX_VALUE)
|
||||
integer.getAndIncrementNoOverflow()
|
||||
assertEquals(integer.get(), Integer.MAX_VALUE)
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,24 @@
|
||||
/* 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.Before
|
||||
import org.junit.Test
|
||||
|
||||
class RunBlockingCounterTest {
|
||||
|
||||
@Before
|
||||
fun setup() {
|
||||
RunBlockingCounter.count.set(0)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `GIVEN we call our custom runBlocking method with counter THEN the latter should increase`() {
|
||||
assertEquals(0, RunBlockingCounter.count.get())
|
||||
runBlockingIncrement {}
|
||||
assertEquals(1, RunBlockingCounter.count.get())
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,39 @@
|
||||
/* 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.detektrules
|
||||
|
||||
import io.gitlab.arturbosch.detekt.api.*
|
||||
import org.jetbrains.kotlin.psi.*
|
||||
import org.jetbrains.kotlin.resolve.BindingContext
|
||||
import org.jetbrains.kotlin.resolve.calls.callUtil.getCall
|
||||
import org.jetbrains.kotlin.resolve.calls.callUtil.getCalleeExpressionIfAny
|
||||
import kotlin.math.exp
|
||||
|
||||
private const val VIOLATION_MSG = "Please use `org.mozilla.fenix.perf.runBlockingImplement` instead" +
|
||||
"because it allows us to monitor the code for performance regressions."
|
||||
|
||||
/**
|
||||
* A check to prevent us from working around mechanisms we implemented in
|
||||
* @see org.mozilla.fenix.perf.RunBlockingCounter.runBlockingIncrement to count how many runBlocking
|
||||
* are used.
|
||||
*
|
||||
* IF YOU UPDATE THIS FILE NAME, UPDATE CODE OWNERS.
|
||||
*/
|
||||
class MozillaRunBlockingCheck(config: Config) : Rule(config) {
|
||||
|
||||
override val issue = Issue(
|
||||
"MozillaRunBlockingCheck",
|
||||
Severity.Performance,
|
||||
"Prevents us from working around mechanisms we implemented to count how many " +
|
||||
"runBlocking are used",
|
||||
Debt.TWENTY_MINS
|
||||
)
|
||||
|
||||
override fun visitImportDirective(importDirective: KtImportDirective) {
|
||||
if (importDirective.importPath?.toString() == "kotlinx.coroutines.runBlocking") {
|
||||
report(CodeSmell(issue, Entity.from(importDirective), VIOLATION_MSG))
|
||||
}
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue