For #11660: added prefetch for topsites and update in onCreateView() #11668

MarcLeclair · 2020-06-17T04:15:04Z

Took the timing for this fix on a g5+

With changes:

Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +614ms (total +1s293ms)
Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +622ms (total +1s304ms)
Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +610ms (total +1s294ms)
Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +597ms (total +1s257ms)
Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +609ms (total +1s270ms)

and without the changes:

Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +911ms (total +1s572ms)
Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +930ms (total +1s558ms)
Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +946ms (total +1s558ms)
Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +793ms (total +1s428ms)
Fully drawn org.mozilla.fennec_aurora/org.mozilla.fenix.HomeActivity: +828ms (total +1s461ms)

I do see a net gain with these changes. It might be worthwhile to implement the same prefetch for collections since it uses the same livedata mechanism 😄

FYI I also did a VIEW (the new app-link name) test with FNPRMS and my results are the same as they are currently (so ~2.9 seconds). This change shouldn't affect it.

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.

After merge

Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

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

codecov-commenter · 2020-06-17T04:38:47Z

Codecov Report

Merging #11668 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##             master   #11668   +/-   ##
=========================================
  Coverage     21.77%   21.78%           
  Complexity      716      716           
=========================================
  Files           375      376    +1     
  Lines         15066    15073    +7     
  Branches       1957     1957           
=========================================
+ Hits           3281     3284    +3     
- Misses        11503    11505    +2     
- Partials        282      284    +2

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/org/mozilla/fenix/FenixApplication.kt	`12.50% <0.00%> (-0.27%)`	`4.00 <0.00> (ø)`
...ava/org/mozilla/fenix/components/TopSiteStorage.kt	`3.33% <0.00%> (-0.24%)`	`1.00 <0.00> (ø)`
...pp/src/main/java/org/mozilla/fenix/ext/LiveData.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...c/main/java/org/mozilla/fenix/home/HomeFragment.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...la/fenix/home/sessioncontrol/SessionControlView.kt	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
.../fenix/browser/browsingmode/BrowsingModeManager.kt	`85.71% <0.00%> (-14.29%)`	`0.00% <0.00%> (ø%)`
...ug/java/org/mozilla/fenix/DebugFenixApplication.kt	`0.00% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
...nix/components/toolbar/BrowserToolbarController.kt	`70.52% <0.00%> (+2.31%)`	`0.00% <0.00%> (ø%)`

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 4270c83...b78ae24. Read the comment docs.

mcomella

I'm concerned about two things:

(I think we should address before landing) On launch, the homescreen flashes on my GS5 as it does an additional layout pass.
(we could land and fast follow-up on this one) We're not blocking for the data the first time so we have non-deterministic behavior which 1) could make instability harder to track down, 2) could confuse our results in FNPRMS, and 3) creates different user experiences that make it harder to trust our experiments (e.g. maybe the difference in behavior changes the outcome of the experiment but it's not something we're measuring).

I tested this on my P2 and GS5 (without measurements) and confirmed that it anecdotally feels faster and the top sites appear at the same time as the rest of the content.

I'd also like to get @csadilek 's feedback because I'm not expert on Fenix code but I want to get these numbers on fnprms to see how they affect startup so I think we should land without the review if we come to a solution for that first issue.

mcomella · 2020-06-17T21:06:13Z

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

+/**
+ * Observe a LiveData once and unregister from it as soon as the live data returns a value
+ */
+fun <T> LiveData<T>.observeOnce(observer: (T) -> Unit) {


nit: observeForeverOnce? It makes it clear it isn't bound to any lifecycle though it's a bit confusing, lol.

Yeah, the name isn't very indicative. I could put observeOnceWithoutLifecycleOwner longer name ,but more self explanatory?

mcomella · 2020-06-17T21:06:52Z

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

+/**
+ * Observe a LiveData once and unregister from it as soon as the live data returns a value
+ */
+fun <T> LiveData<T>.observeOnce(observer: (T) -> Unit) {


nit: this observer name is redundant to the anonymous observer we create. Maybe callback or simple function?

mcomella · 2020-06-17T21:12:22Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+        // This has to be called separately from the consumeFrom block since the coroutine doesn't
+        // allow our UI to be updated right away and delays our start up. The init block allows us to
+        // force our UI to render with the data available in our store at fragment creation time.
+        sessionControlView?.update(homeFragmentStore.state)


nit: move next to consumeFrom block you're referecing.

Also, it might be helpful to encapsulate them both in a helper function so the grouping is clear:

fun updateSessionControlView() { // maybe a better name fun update(state) { sessionControlView?.update(state) } // explain update now update(homeFragmentStore.state) // explain update later addObserver(::update) }

Two best practices in play here:

when I'm adding code with comments, I always think if it'd be more appropriate to put the code in a function to encapsulate the complexity

when I'm adding code to lifecycle methods, I always think about using a helper to keep the lifecycle methods short and discussing things at a high level (this doesn't seem to be common on Fenix but I think it greatly helps clarity)

mcomella · 2020-06-17T21:16:06Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

@@ -215,6 +215,12 @@ class HomeFragment : Fragment() {
            sessionControlInteractor,
            homeViewModel
        )
+
+        // This has to be called separately from the consumeFrom block since the coroutine doesn't


nit: this is unclear. Which consumeFrom block (see my comment below)? What coroutine? What do you mean by "delays our start up"? How? What init block? How does it force our UI to render?

mcomella · 2020-06-17T21:17:32Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+        // This has to be called separately from the consumeFrom block since the coroutine doesn't
+        // allow our UI to be updated right away and delays our start up. The init block allows us to
+        // force our UI to render with the data available in our store at fragment creation time.
+        sessionControlView?.update(homeFragmentStore.state)


Big issue: what happens if the data isn't available when this is called? We might want to consider blocking on it to make this more deterministic (e.g. if our fnprms starts reporting two drastically different numbers randomly, we'll be confused).

I confirmed this isn't deterministic as is: when running debug builds on Pixel 2, I see the top sites animation.

The list should be empty and will just update itself as its doing now. we could block on it too if needed

mcomella · 2020-06-17T21:24:49Z

app/src/main/java/org/mozilla/fenix/components/TopSiteStorage.kt

@@ -86,4 +87,10 @@ class TopSiteStorage(private val context: Context) {
            context.settings().defaultTopSitesAdded = true
        }
    }
+
+    fun prefetch() {


nit: I think we need to explain why we do this. We should probably only do so once in the code so maybe say to reference some other method (or the other explanation should reference here?)

We should probably also explain why this is innocuous.

mcomella · 2020-06-17T21:25:37Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+        // This has to be called separately from the consumeFrom block since the coroutine doesn't
+        // allow our UI to be updated right away and delays our start up. The init block allows us to
+        // force our UI to render with the data available in our store at fragment creation time.
+        sessionControlView?.update(homeFragmentStore.state)


Big issue: I don't love that we're "setting top sites twice" but only dispatching the change once but it seems innocuous on P2. However, on my GS5, the whole homescreen flashes, presumably because it relays out the RV after this line: https://searchfox.org/mozilla-mobile/rev/d81a3c3fce58d3a6c3ebd3257ad1566c9bd462ef/fenix/app/src/main/java/org/mozilla/fenix/home/sessioncontrol/SessionControlView.kt#147 The current behavior does this too but it doesn't look as bad because top sites gets added instead of just replaced.

I'm not sure what we should do about that... Let's discuss

Interesting. I don't see it on my G5. We could do a quick "if this is the first call from the consumeFrom, ignore"

The problem is fixed if we remove that line. The line was added in #5591 and was trying to fix #2874, #5131, and #5375. The first two relate to problems with tabs updating on the homescreen – no longer a problem – and I'm unable to reproduce the 3rd issue (maybe also tabs related)? I tried reproducing similar issues with collections:

open homescreen, open tabs tray, click 3-dot to add to collection, see homescreen

open history/bookmarks, add collection, open homescreen

But I can't reproduce. I think this is safe to remove but we should check with ekager.

mcomella · 2020-06-17T21:28:14Z

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

@@ -229,6 +230,13 @@ open class FenixApplication : LocaleAwareApplication() {
        // no-op, LeakCanary is disabled by default
    }

+    // This is for issue https://github.com/mozilla-mobile/fenix/issues/11660. We prefetch our info for startup


I think it's confusing that we say, "our info" but really mean "top sites" so I'd rather say "We prefetch our top sites for startup" and it can be changed if the code becomes more general later.

mcomella · 2020-06-17T21:28:22Z

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

+        StrictMode.allowThreadDiskReads().resetPoliciesAfter {
+            components.core.topSiteStorage.prefetch()
+        }
+    }
    private fun setupPush() {


nit: whitespace above

mcomella · 2020-06-17T21:28:44Z

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

+    // This is for issue https://github.com/mozilla-mobile/fenix/issues/11660. We prefetch our info for startup
+    // so that we're sure that we have all the data available as our fragment is launched.
+    private fun prefetchForHomeFragment() {
+        StrictMode.allowThreadDiskReads().resetPoliciesAfter {


Is this necessary? I don't see anywhere that would cause us to read on the main thread but perhaps I misunderstand.

Not sure why, but our strictmodes complained when I didn't add it and the CI tests failed ( some of them). I could try to remove it

mcomella

I think we should land as is in order to get these numbers from fnprms and see we're at. I think we should also get retroactive reviews from csadilek and ekager and follow-up with a few PRs:

Make it deterministic
Address the nits

mcomella · 2020-06-18T00:17:26Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+     * The [SessionControlView] is forced to update with our current state when we call [HomeFragment.onCreateView]
+     * in order to be able to draw everything at once with the current data in our store. The [View.consumeFrom] coroutine dispatch
+     * doesn't get run right away which means that we won't draw on the first layout pass.
+     * */


nit: */, not * */

mcomella · 2020-06-18T00:17:54Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

@@ -240,6 +234,19 @@ class HomeFragment : Fragment() {
        return view
    }

+    /**
+     * The [SessionControlView] is forced to update with our current state when we call [HomeFragment.onCreateView]


nit: why do we need to draw everything at once? Make it clear this is a perf issue

mcomella · 2020-06-18T00:18:41Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

+     * doesn't get run right away which means that we won't draw on the first layout pass.
+     * */
+    fun updateSessionControlView(view: View) {
+        sessionControlView?.update(homeFragmentStore.state)


nit: I like the inner function version I suggested because it makes it clear that the code is repeated and prevents someone from missing that if they decide to refactor this. But I can go either way

mcomella · 2020-06-18T00:19:15Z

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt

@@ -216,16 +216,10 @@ class HomeFragment : Fragment() {
            homeViewModel


nit: In the extended commit summary, I would have included information on why it's okay to remove this code (though I guess it's fine if it's just in the PR text? maybe you can link to what's in the PR).

TopSites will be prefetched with observerOnce (wrapper around observerForever). Also, the SessionControlView.update() is called right away instead of waiting from consumeFrom in the HomeFragment.onCreateView() which will allow the UI to render all at once on its first perform traversal

…end device

…illa-mobile#11821)" This reverts commit bd7a537.

This reverts commit bd7a537.

MarcLeclair requested a review from mcomella June 17, 2020 04:15

MarcLeclair force-pushed the 11660_prefetch_topsites branch 2 times, most recently from 7be628c to 82d5296 Compare June 17, 2020 17:48

mcomella reviewed Jun 17, 2020

View reviewed changes

mcomella requested a review from csadilek June 17, 2020 21:52

mcomella approved these changes Jun 18, 2020

View reviewed changes

MarcLeclair force-pushed the 11660_prefetch_topsites branch 6 times, most recently from 06956bc to d07021d Compare June 18, 2020 17:30

MarcLeclair added 2 commits June 18, 2020 19:09

Removed the submitList(null) since it retriggered a drawing on lower …

b78ae24

…end device

MarcLeclair force-pushed the 11660_prefetch_topsites branch from d07021d to b78ae24 Compare June 18, 2020 23:12

MarcLeclair merged commit b52091e into mozilla-mobile:master Jun 18, 2020

MarcLeclair added a commit to MarcLeclair/fenix that referenced this pull request Jun 22, 2020

For 11660: fixing nits for previous mozilla-mobile#11668

64d627e

MarcLeclair mentioned this pull request Jun 22, 2020

For 11660: fixing nits for previous #11668 #11821

Merged

4 tasks

liuche mentioned this pull request Jun 27, 2020

Releng 78.0.1/79.0.0 #12048

Closed

12 tasks

ekager mentioned this pull request Jul 29, 2020

For #12642 - Use shared prefs for important values of top sites and collections to avoid flash #13009

Closed

4 tasks

MarcLeclair added a commit to MarcLeclair/fenix that referenced this pull request Aug 25, 2020

For 11660: fixing nits for previous mozilla-mobile#11668

3d73339

MarcLeclair added a commit that referenced this pull request Aug 26, 2020

For 11660: fixing nits for previous #11668 (#11821)

bd7a537

csadilek added a commit to csadilek/fenix that referenced this pull request Aug 26, 2020

Revert "For 11660: fixing nits for previous mozilla-mobile#11668 (moz…

0f2582c

…illa-mobile#11821)" This reverts commit bd7a537.

csadilek added a commit to csadilek/fenix that referenced this pull request Aug 26, 2020

Revert "For 11660: fixing nits for previous mozilla-mobile#11668 (moz…

b9eddd5

…illa-mobile#11821)" This reverts commit bd7a537.

jonalmeida pushed a commit that referenced this pull request Aug 26, 2020

Revert "For 11660: fixing nits for previous #11668 (#11821)"

0162fd6

This reverts commit bd7a537.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #11660: added prefetch for topsites and update in onCreateView() #11668

For #11660: added prefetch for topsites and update in onCreateView() #11668

MarcLeclair commented Jun 17, 2020 •

edited

Loading

codecov-commenter commented Jun 17, 2020 •

edited

Loading

mcomella left a comment

mcomella Jun 17, 2020

MarcLeclair Jun 17, 2020

mcomella Jun 17, 2020

mcomella Jun 17, 2020

mcomella Jun 17, 2020

mcomella Jun 17, 2020

MarcLeclair Jun 17, 2020

mcomella Jun 17, 2020

mcomella Jun 17, 2020

MarcLeclair Jun 17, 2020

mcomella Jun 17, 2020

mcomella Jun 17, 2020

mcomella Jun 17, 2020

mcomella Jun 17, 2020

MarcLeclair Jun 17, 2020

mcomella left a comment

mcomella Jun 18, 2020

mcomella Jun 18, 2020

mcomella Jun 18, 2020

mcomella Jun 18, 2020

		@@ -216,16 +216,10 @@ class HomeFragment : Fragment() {
		homeViewModel

For #11660: added prefetch for topsites and update in onCreateView() #11668

For #11660: added prefetch for topsites and update in onCreateView() #11668

Conversation

MarcLeclair commented Jun 17, 2020 • edited Loading

Pull Request checklist

After merge

To download an APK when reviewing a PR:

codecov-commenter commented Jun 17, 2020 • edited Loading

Codecov Report

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

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

MarcLeclair commented Jun 17, 2020 •

edited

Loading

codecov-commenter commented Jun 17, 2020 •

edited

Loading