15278 detekt rule runblocking #15942

MarcLeclair · 2020-10-15T22:08:23Z

Added a detekt rule for runBlocking since we noticed that it can affect performance at times. So I switched runBlocking to runBlockingCounter (which just wraps the runBlocking call) so that we can monitor the # of times it is being called (through tests and other means)

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

… calls

…d its config to the yaml

…st.kt to PerformanceStartupTest.kt and added runBlockingCount test

mcomella

This looks pretty good – I have fair number of comments but they're mostly nits. Requesting to see the changes to make sure the naming seems sound.

app/src/main/java/org/mozilla/fenix/CoroutineManager.kt

app/src/test/java/org/mozilla/fenix/library/bookmarks/DesktopFoldersTest.kt

mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaRunblockingCheck.kt

mcomella · 2020-10-16T17:23:04Z

app/src/androidTest/java/org/mozilla/fenix/ui/PerformanceStartupTest.kt

+    is the optimal solution.
+
+"""
+
 /**
 * A performance test to limit the number of StrictMode suppressions on startup.
 * This test was written by the perf team.


We changed the file name: we need to update CODEOWNERS.

mcomella · 2020-10-16T17:26:50Z

config/detekt.yml

@@ -117,6 +117,8 @@ mozilla-detekt-rules:
    excludes: "**/*Test.kt, **/*Spec.kt, **/test/**, **/androidTest/**"
  MozillaCorrectUnitTestRunner:
    active: true
+  MozillaRunblockingCheck:


Don't forget to update this name if you rename this

mcomella · 2020-10-16T17:29:45Z

app/src/main/java/org/mozilla/fenix/RunBlockingCounter.kt

+@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+class RunBlockingCounter {
+    companion object {
+        var runBlockingCount = 0L


Given this runs in production code, I'm starting to think we should prevent overflow. The app shouldn't have a hard limit on how long it can run. Maybe in the setter? We could probably change it back to an Int in that case.

I can always add it to the runBlockingIncrement function. I just felt uneasy about adding more functionality (albeit its probably minimal) to a function that's just suppose to wrap one provided by Kotlin

Why do you want to avoid adding too much functionality to a wrapping method?

mcomella · 2020-10-16T17:33:01Z

app/src/main/java/org/mozilla/fenix/RunBlockingCounter.kt

+/**
+ * Wrapper around `runBlocking`
+ */
+fun <T> runBlockingIncrement(


I don't love this name. While it's telling us what it does, it's also kind of vague – increment what?

Naming this seems really hard. The intentions I can think of are:

tell them what it does (*increment, *perf, *monitor, ...)

tell them it's preferred (*safe, *preferred, ...)

Maybe runBlockingMonitored? It sounds intimidating so maybe people will think twice before using it. 🤣 Or maybe they'll think we're the perf 👮‍♀️ :(

I guess that can work. I was thinking that 1) if the kdoc explains it further in details, then a looser name could be fine? Or 2) we could go longer, but more precise such as runBlockingPerformanceTracker? It seems a bit less intimidating and its purpose is conveyed a lot more from the name.

I think loose names are only okay if the function is frequently used, e.g. map or add. Given this is a function people aren't supposed to call a lot anyway, I think it's fine to have a longer name. However, I wanted to avoid making it too tied to performance so people don't think, "perf police!"

mcomella · 2020-10-16T17:36:01Z

app/src/main/java/org/mozilla/fenix/RunBlockingCounter.kt

+@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+class RunBlockingCounter {
+    companion object {
+        var runBlockingCount = 0L


This needs to be synchronized (if it's synchronized, does it automatically get volatile too? I'm not sure)

If I add @Synchronize set for this variable, isn't there an issue where Thread 1 --> Increases counter, then runs its method, returns and Thread 2 --> waits for the method to return before increasing? Although I don't think that would happen, since the lock would be released as soon as we're done incrementing. I guess the only issue would be if a billion threads queue'd up at the same time to increment the counter, which I think is unlikely.

Maybe I'm overthinking it though, I added the @Synchronized with my latest fix. I saw nothing locally but it was just a thought.

Yes, that's possible. I'm concerned that the value will not be accessed correctly in concurrent situations, however.

Actually, I don't know how the java memory model works. Is assignment atomic? In that case, I don't think we need synchronized, just volatile. We could also change it to an AtomicInteger (or whatever) to to avoid having to figure it out.

From what I remember, if the interger is volatile it is kept in the main memory and access to that integer is the same throughout different threads. I think, however, that AtomicInteger can solve this issue

@mcomella So I added AtomicInteger but I also think we need to synchronized(...) on the check for overflow since two threads my overlap. what do you think?

…moved RunBlockingCounter to perf package

…nBlockingCheck

…without restting counter

…kingIncrement

…o count setter

mcomella

Pretty close but we still have a few things to iron out such as:

concurrency
clarity of logic

mcomella · 2020-10-21T23:43:17Z

app/src/androidTest/java/org/mozilla/fenix/ui/StartupExcessiveResourceUseTest.kt

@@ -39,20 +54,23 @@ private const val FAILURE_MSG = """StrictMode startup suppression count does not
 *
 * IF YOU UPDATE THE TEST NAME, UPDATE CODE OWNERS.


Did you read the comment? 😝 Can you add this to code owners?

mcomella · 2020-10-21T23:44:03Z

app/src/androidTest/java/org/mozilla/fenix/ui/StartupExcessiveResourceUseTest.kt

+    is the optimal solution.
+
+"""
+
 /**
 * A performance test to limit the number of StrictMode suppressions on startup.


nit: update the comment

mcomella · 2020-10-21T23:45:01Z

app/src/main/java/org/mozilla/fenix/components/searchengine/FenixSearchEngineProvider.kt

-                    null
+    fun installedSearchEngines(context: Context): SearchEngineList =
+        runBlockingIncrement {
+            val installedIdentifiers = installedSearchEngineIdentifiers(context)


nit: can we avoid breaking blame? Or is it impossible because the line is too long?

mcomella · 2020-10-21T23:45:16Z

app/src/main/java/org/mozilla/fenix/components/searchengine/FenixSearchEngineProvider.kt

-        loadedSearchEngines.await().list.map { it.identifier }
-    }
+    fun allSearchEngineIdentifiers() =
+        runBlockingIncrement {


nit: avoid blame

mcomella · 2020-10-21T23:45:43Z

app/src/main/java/org/mozilla/fenix/components/searchengine/FenixSearchEngineProvider.kt

        if (isCustom) {
            CustomSearchEngineStore.removeSearchEngine(context, searchEngine.identifier)
            reload()
        } else {
            val installedIdentifiers = installedSearchEngineIdentifiers(context).toMutableSet()
            installedIdentifiers.remove(searchEngine.identifier)
-            prefs(context).edit().putStringSet(localeAwareInstalledEnginesKey(), installedIdentifiers).apply()
+            prefs(context).edit()


nit: you didn't change this line so don't break blame

mcomella · 2020-10-21T23:48:06Z

config/detekt.yml

@@ -117,6 +117,9 @@ mozilla-detekt-rules:
    excludes: "**/*Test.kt, **/*Spec.kt, **/test/**, **/androidTest/**"
  MozillaCorrectUnitTestRunner:
    active: true
+  MozillaRunBlockingCheck:
+    active: true
+    excludes: "**/*Test.kt"


nit: we should copy the same exclusion blocks as all the others. e.g. we don't want to enforce this in androidTest either.

mcomella · 2020-10-21T23:49:12Z

app/src/main/java/org/mozilla/fenix/perf/RunBlockingCounter.kt

+@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+object RunBlockingCounter {
+    var count = 0L
+        @Synchronized set


As per my earlier comment, this might just need to be volatile – can you check how java assignment works? Is += atomic? Maybe we need AtomicInteger to increment.

mcomella · 2020-10-21T23:50:33Z

app/src/main/java/org/mozilla/fenix/perf/RunBlockingCounter.kt

+ */
+@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+object RunBlockingCounter {
+    var count = 0L


Handle overflow? fwiw, I like handling it here, in the setter, rather than in runBlockingIncrement because it keeps the other method simple. That being said, it might not be possible with AtomicInteger.

mcomella · 2020-10-21T23:54:06Z

app/src/main/java/org/mozilla/fenix/perf/RunBlockingCounter.kt

+): T {
+    RunBlockingCounter.count += 1
+
+    context?.let {


This doesn't do what you think it does. It does:

if (context != null) { val returnValue = runBlocking(context) { action() } if (returnValue != null) { runBlocking(action()) } }

I guess unless return return from the containing function and not the lambda. Even if the code is correct, the fact that it can be misinterpreted isn't great. In general, it's good to be careful when using symbols (?:) or unintuitively short functions (let, run, and friends) to express something.

I would prefer

return if (context != null) { runBlocking(it, ::action) } else { runBlocking(::action) }

Note that the function reference is an optimization that avoids us having to create a new lambda (if the compiler isn't smart enough to do it for us). Note also the deduplication of the return statement.

mcomella · 2020-10-21T23:58:19Z

mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaRunBlockingCheck.kt

+import kotlin.math.exp
+
+private const val VIOLATION_MSG = "Please use `org.mozilla.fenix.perf.RunBlockingCounter" +
+        ".fenixRunBlocking` instead because it allows us to monitor the code for performance " +


this needs to be updated. Remember, the file name won't be used.

codecov-io · 2020-10-27T21:37:28Z

Codecov Report

Merging #15942 into master will increase coverage by 0.02%.
The diff coverage is 60.00%.

@@             Coverage Diff              @@
##             master   #15942      +/-   ##
============================================
+ Coverage     29.99%   30.02%   +0.02%     
- Complexity     1204     1207       +3     
============================================
  Files           451      453       +2     
  Lines         18513    18525      +12     
  Branches       2546     2549       +3     
============================================
+ Hits           5553     5562       +9     
- Misses        12531    12532       +1     
- Partials        429      431       +2

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/org/mozilla/fenix/FenixApplication.kt	`7.45% <0.00%> (ø)`	`5.00 <0.00> (ø)`
...la/fenix/customtabs/FennecWebAppIntentProcessor.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...a/fenix/settings/search/AddSearchEngineFragment.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...mponents/searchengine/FenixSearchEngineProvider.kt	`52.63% <33.33%> (ø)`	`17.00 <2.00> (+1.00)`
.../java/org/mozilla/fenix/perf/RunBlockingCounter.kt	`71.42% <71.42%> (ø)`	`2.00 <2.00> (?)`
...c/main/java/org/mozilla/fenix/ext/AtomicInteger.kt	`80.00% <80.00%> (ø)`	`0.00 <0.00> (?)`
...a/fenix/components/history/PagedHistoryProvider.kt	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...la/fenix/components/metrics/AppStartupTelemetry.kt	`97.72% <100.00%> (ø)`	`23.00 <1.00> (ø)`
app/src/main/java/org/mozilla/fenix/ext/String.kt	`72.50% <100.00%> (ø)`	`0.00 <0.00> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b65c798...e0be45d. Read the comment docs.

…DEOWNERS

mcomella · 2020-10-28T20:56:57Z

app/src/main/java/org/mozilla/fenix/perf/RunBlockingCounter.kt

+    context: CoroutineContext? = null,
+    action: suspend CoroutineScope.() -> T
+): T {
+    synchronized(lock = Any()) {


This won't synchronized properly: each time we go through this, we lock a new instance so there is never any contention for this code block. You need to use a shared instance.

That being said, do we know that this class throws an exception on overflow? Maybe it just does something safe.

If not, I think the synchronization block could be replaced with something like getAndUpdate: https://developer.android.com/reference/java/util/concurrent/atomic/AtomicInteger?hl=en#getAndUpdate(java.util.function.IntUnaryOperator)

@mcomella so getAndUpdate is min API 24. Also, the AtomicInteger seems to wrap around like a regular integer. We could just have a function in the object and let it handle it itself?

mcomella

The code looks good to me but I'd like some of the nits addressed. If we change incrementCounter to an extension function as I suggested, it might be good to flag me for re-review on that one specific section.

mcomella · 2020-10-29T22:50:31Z

app/src/androidTest/java/org/mozilla/fenix/ui/StartupExcessiveResourceUseTest.kt

-        val actual = activityTestRule.activity.components.strictMode.suppressionCount.toInt()
-        assertEquals(FAILURE_MSG, EXPECTED_SUPPRESSION_COUNT, actual)
+        val actualSuppresionCount = activityTestRule.activity.components.strictMode.suppressionCount.toInt()
+        val actualRunBlocking = RunBlockingCounter.count


This is an AtomicInteger now – we need to call get() before doing the comparison.

mcomella · 2020-10-29T22:50:52Z

app/src/main/java/org/mozilla/fenix/FenixApplication.kt

@@ -137,7 +137,9 @@ open class FenixApplication : LocaleAwareApplication(), Provider {
            // to invoke parts of itself that require complete megazord initialization
            // before that process completes, we wait here, if necessary.
            if (!megazordSetup.isCompleted) {
-                runBlocking { megazordSetup.await(); }
+                runBlockingIncrement {


nit: can we keep this as one line? I want to avoid these changes "double-spacing" the code.

mcomella · 2020-10-29T22:51:38Z

app/src/main/java/org/mozilla/fenix/components/searchengine/FenixSearchEngineProvider.kt

-                } else {
-                    null
+    fun installedSearchEngines(context: Context): SearchEngineList = runBlockingIncrement {
+            val installedIdentifiers = installedSearchEngineIdentifiers(context)


nit: Blame is still broken here: it's double-indented. I think it'd be fixed if we corrected the indentation.

mcomella · 2020-10-29T22:51:52Z

app/src/main/java/org/mozilla/fenix/components/searchengine/FenixSearchEngineProvider.kt

-        loadedSearchEngines.await().list.map { it.identifier }
-    }
+    fun allSearchEngineIdentifiers() = runBlockingIncrement {
+            loadedSearchEngines.await().list.map { it.identifier }


nit: same indentation issues here and below.

mcomella · 2020-10-29T22:53:22Z

app/src/test/java/org/mozilla/fenix/RunBlockingCounterTest.kt

+ * 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


nit: move to org.mozilla.fenix.perf to match the production code

mcomella · 2020-10-29T22:55:16Z

mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaRunBlockingCheck.kt

+import org.jetbrains.kotlin.resolve.calls.callUtil.getCalleeExpressionIfAny
+import kotlin.math.exp
+
+private const val VIOLATION_MSG = "Please use `org.mozilla.fenix.perf.RunBlockingCounter` instead " +


nit: This function name is outdated.

mcomella · 2020-10-29T23:06:47Z

app/src/main/java/org/mozilla/fenix/perf/RunBlockingCounter.kt

+object RunBlockingCounter {
+    var count = AtomicInteger(0)
+
+    fun incrementCounter() {


nit: I think it's simpler to move this to be an extension function, e.g. fun AtomicInteger.getAndIncrementNoOverflow(). It lends itself to better testing (why isn't this method tested? :P), re-use, and more greatly discourages touching any code outside of the AtomicInteger, unlike a function in a companion object.

mcomella · 2020-10-29T23:10:28Z

app/src/main/java/org/mozilla/fenix/perf/RunBlockingCounter.kt

+        do {
+            prev = count.get()
+            next = prev + 1
+            if (next == Integer.MAX_VALUE) next = 0


nit: while we're not going to hit this case, I think it's more honest to cap it at Integer.MAX_VALUE rather than reset the counter. e.g. if you look at the counter after running the program after a few days and its 100, you might be a little confused.

mcomella · 2020-10-29T23:10:41Z

app/src/main/java/org/mozilla/fenix/perf/RunBlockingCounter.kt

+ */
+@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+object RunBlockingCounter {
+    var count = AtomicInteger(0)


mcomella · 2020-10-29T23:11:52Z

app/src/main/java/org/mozilla/fenix/perf/RunBlockingCounter.kt

+/**
+ * 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 have negative


nit: "negative side-effects on our main thread"? Maybe we should just say, "block the main thread"

…ion and fixed indentation

mcomella

Some brief clean up changes – these could land as a follow-up so we can get this landed.

Note that you'll also have to rebase.

mcomella · 2020-10-31T00:16:38Z

app/src/main/java/org/mozilla/fenix/ext/AtomicInteger.kt

+/**
+ *  Increases an AtomicInteger safely.
+ */
+fun AtomicInteger.getAndIncrementNoOverflow(counter: AtomicInteger) {


You shouldn't pass it as an argument – you should use this instead.

mcomella · 2020-10-31T00:17:04Z

app/src/main/java/org/mozilla/fenix/ext/AtomicInteger.kt

+/**
+ *  Increases an AtomicInteger safely.
+ */
+fun AtomicInteger.getAndIncrementNoOverflow(counter: AtomicInteger) {


Can we write tests for this?

mcomella · 2020-10-31T00:18:27Z

mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaRunBlockingCheck.kt

+import org.jetbrains.kotlin.resolve.calls.callUtil.getCalleeExpressionIfAny
+import kotlin.math.exp
+
+private const val VIOLATION_MSG = "Please use `org.mozilla.fenix.perf.RunBlockingCounter.runBlockingIncrement`" +


It's org.mozilla.fenix.perf.runBlockingImplement – top level functions don't include the file name in their fully-qualified class name.

…d nit fix

MarcLeclair added 6 commits October 15, 2020 17:13

For mozilla-mobile#15278: added CoroutineManager to count runBlocking…

266786b

… calls

For mozilla-mobile#15278: Added actual detekt rule for runblocking an…

da7b1ed

…d its config to the yaml

For mozilla-mobile#15278: Added unit test for RunblockingCounter

1214bb6

For mozilla-mobile#15278: renamed StrictModeStartupSuppressionCountTe…

3fd69e2

…st.kt to PerformanceStartupTest.kt and added runBlockingCount test

Lint fix

d526980

For mozilla-mobile#15278: made runblocking a Long to prevent overflow

d1db99f

MarcLeclair requested a review from mcomella October 15, 2020 22:08

MarcLeclair requested review from a team as code owners October 15, 2020 22:08

liuche added the needs:review PRs that need to be reviewed label Oct 15, 2020

gabrielluong assigned mcomella Oct 15, 2020

mcomella suggested changes Oct 16, 2020

View reviewed changes

MarcLeclair added 7 commits October 20, 2020 14:52

For mozilla-mobile#15278: fixed MozRunblocking name, description and …

5f1c747

…moved RunBlockingCounter to perf package

For mozilla-mobile#15278:Renamed MozillaRunblockingCheck to MozillaRu…

c277469

…nBlockingCheck

For mozilla-mobile#15278: Added setup for unit test, since it failed …

de6a96d

…without restting counter

For mozilla-mobile#15278: Fixed naming for RunBlocking lint check

745e9af

For mozilla-mobile#15278: removed changes made to test to use runBloc…

da0f4c6

…kingIncrement

For mozilla-mobile#15728: added test exclusion for runBlocking check

bb5636d

For mozilla-mobile#15278: changed null check and added Synchronized t…

a121a9b

…o count setter

MarcLeclair requested a review from mcomella October 21, 2020 00:01

mcomella suggested changes Oct 21, 2020

View reviewed changes

MarcLeclair requested a review from mcomella October 27, 2020 20:50

For mozilla-mobile#15278: fix for nits

8d63ebc

For mozilla-mobile#15278: added StartupExcessiveResourceUseTest to CO…

36fc3e1

…DEOWNERS

mcomella reviewed Oct 28, 2020

View reviewed changes

For mozilla-mobile#15278: fixed for nits

676fb6b

MarcLeclair requested a review from mcomella October 29, 2020 19:10

mcomella approved these changes Oct 29, 2020

View reviewed changes

For mozilla-mobile#15278: Moved increment function to extension funct…

5f2da80

…ion and fixed indentation

mcomella mentioned this pull request Oct 31, 2020

Avoid unnecessary initialization in nested lazy initializers #15279

Closed

mcomella reviewed Oct 31, 2020

View reviewed changes

MarcLeclair added 2 commits November 2, 2020 01:26

For mozilla-mobile#15278: Added tests for Atomic Integer extension an…

b265a37

…d nit fix

merged master

e0be45d

MarcLeclair merged commit 7b1af41 into mozilla-mobile:master Nov 2, 2020

		@@ -39,20 +54,23 @@ private const val FAILURE_MSG = """StrictMode startup suppression count does not
		*
		* IF YOU UPDATE THE TEST NAME, UPDATE CODE OWNERS.

15278 detekt rule runblocking #15942

15278 detekt rule runblocking #15942

Conversation

MarcLeclair commented Oct 15, 2020

Pull Request checklist

To download an APK when reviewing a PR:

mcomella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcomella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 27, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcomella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcomella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 27, 2020 •

edited