789887 - Multiple successive calls to getTopSites() in AboutHomeContent

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

I've been investigating slowness in our top sites query for the awesome screen in bug 785945, and that led me to find that on startup on about:home we end up kicking off *3* top sites queries, which sounds like something we should avoid.

Comment 1

[:Margaret Leibovic]

Looks like this is happening because onActivityContentChanged is getting called multiple times, which we call from BrowserApp.onContentChanged().

Lucas, I see you in the blame history here, so do you remember why we chose this place to call loadTopSites? I feel like we need to figure out a way to only get the cursor once per time the page is shown.

Comment 2

[Lucas Rocha (:lucasr)]

(In reply to Margaret Leibovic [:margaret] from comment #1)
> Looks like this is happening because onActivityContentChanged is getting
> called multiple times, which we call from BrowserApp.onContentChanged().
> 
> Lucas, I see you in the blame history here, so do you remember why we chose
> this place to call loadTopSites? I feel like we need to figure out a way to
> only get the cursor once per time the page is shown.

IIRC, I simply kept this call in onContentChanged() when I implemented the final design for about:home. Brad was the one who added this call in the original code. Feels like this call could be removed now as I made about:home get updated every time it's shown, see:

  https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#618

Comment 3

[:Margaret Leibovic]

Actually, upon further investigation, I found that the very first one of these loadTopSites calls came from finishProfileMigration, which seems like it might not be necessary, but that only happens on first run, so that's not our worst problem for now.

Then I'm seeing another 3 come from that AboutHomeRunnable call. I added some more logging and found that those come from initializeChrome, then the location change tab event and the selected tab event :/

I think the simplest thing to do here is to just cache the cursor and not run the query more than every 30 seconds or so. However, that means we'd also need to add some sort of force-query flag for things like the call to update top sites after history has been cleared.

Comment 4

[:Margaret Leibovic]

Attachment: WIP

I'm not sure that I like adding a force parameter like I did in here, but I'm not sure if there's a better way to do this without refactoring the way we handle the different UpdateFlags.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 659688 [details] [diff] [review]
WIP

Review of attachment 659688 [details] [diff] [review]:
-----------------------------------------------------------------

We still get the multiple successive queries even if you remove the onContentChanged() callback? The timeout code is ok-ish but I'd prefer a solution where we only re-run the query when necessary.

::: mobile/android/base/AboutHomeContent.java
@@ +294,5 @@
>              }
>          });
>      }
>  
> +    void update(final EnumSet<UpdateFlags> flags, final boolean force) {

I'd probably just add an UpdateFlags.FORCE instead of this extra argument here.

@@ +299,4 @@
>          GeckoAppShell.getHandler().post(new Runnable() {
>              public void run() {
>                  if (flags.contains(UpdateFlags.TOP_SITES))
> +                    loadTopSites(force);

Then this would become loadTopSites(flags.contains(UpdateFlags.FORCE))

Comment 6

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #5)

> We still get the multiple successive queries even if you remove the
> onContentChanged() callback? The timeout code is ok-ish but I'd prefer a
> solution where we only re-run the query when necessary.

Yeah, see comment 3. I agree that it would be good to try just to re-run the query when necessary, but that would involve refactoring the way we update about:home to decouple updating the cursor from updating the UI, since right now those concepts are bundled together in showAboutHome(). This problem traces its way back to calling showAboutHome() from different places, like when we get a location change event and a tab select event for the first tab on startup. I suppose the best solution would make it so that we only call showAboutHome() once in any given situation when we show it, which perhaps we can do by checking if it's already showing. I can investigate that, since that would prevent making a whole new AboutHomeRunnable when it's not necessary.

> ::: mobile/android/base/AboutHomeContent.java
> @@ +294,5 @@
> >              }
> >          });
> >      }
> >  
> > +    void update(final EnumSet<UpdateFlags> flags, final boolean force) {
> 
> I'd probably just add an UpdateFlags.FORCE instead of this extra argument
> here.

I was thinking that, but then we'd have to change UpdateFlags.ALL. I guess that's not a big deal, though. Definitely more elegant.

Comment 7

[:Margaret Leibovic]

Attachment: avoid creating unnecessary AboutHomeRunnables

This is simpler. As a I mentioned in comment 6, the root problem is really that we create multiple AboutHomeRunnables that aren't necessary. This should save us from doing that extra work.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 663555 [details] [diff] [review]
avoid creating unnecessary AboutHomeRunnables

Review of attachment 663555 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: mobile/android/base/BrowserApp.java
@@ +604,5 @@
>          Runnable r = new AboutHomeRunnable(false);
>          mMainHandler.postAtFrontOfQueue(r);
>      }
>  
> +    private class AboutHomeRunnable implements Runnable {

Seems unrelated.

Comment 9

[:Margaret Leibovic]

(In reply to Lucas Rocha (:lucasr) from comment #8)

> ::: mobile/android/base/BrowserApp.java
> @@ +604,5 @@
> >          Runnable r = new AboutHomeRunnable(false);
> >          mMainHandler.postAtFrontOfQueue(r);
> >      }
> >  
> > +    private class AboutHomeRunnable implements Runnable {
> 
> Seems unrelated.

Yeah, I guess it's a little scope-creepy, but I took this opportunity to clean up the access level of these methods/class to restrict them to what's actually needed.

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1003f039c77

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c1003f039c77

Comment 12

[Aaron Train [:aaronmt]]

Backed out in bug 794731.

Comment 13

[Robert Longson [:longsonr]]

Shouldn't mAboutHomeShowing be initialised to something in a constructor?

Comment 14

[:Margaret Leibovic]

Attachment: patch v2

The problem that caused bug 794731 was that we depend on hiding the about:home content in the onTabChanged handler. We weren't hiding it because mAboutHomeShowing was never set to true, so it was hanging around and obscuring the normal content view.

This also makes me realize that right now we're creating/running a new AboutHomeRunnable on every location change event, which will be nice to avoid.

I experimented with making AboutHomeContent hidden by default in gecko_app, but that led to some ugly flickering when launching the app regularly, so I think this approach is better.

Comment 15

[:Margaret Leibovic]

(In reply to Robert Longson from comment #13)
> Shouldn't mAboutHomeShowing be initialised to something in a constructor?

In Java, a primitive boolean defaults to false.

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 665951 [details] [diff] [review]
patch v2

Review of attachment 665951 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch.

Comment 17

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57c89d10f0ab

(I confirmed this fixes bug 794731 before landing)

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/57c89d10f0ab