925163 - Favicons are slow to load when scrolling home list views

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[:Margaret Leibovic]

The saga continues! I've noticed significant slowness on Nightly since bug 922116 landed, but this may actually be fallout from the original work that was done in bug 905685.

I know that we're hoping issues like this will all be fixed when bug 914296 lands, but if this slowness is currently on Aurora, I think we need to come up with a different solution.

Has anyone else been seeing this?

Comment 1

[Richard Newman [:rnewman]]

Depends what you call "slow". Favicon loads keep up with a page-by-page inertial scroll when I swipe. Definitely nowhere near instantaneous, but of course instantaneous would come with its own cost.

I don't recall what the behavior was like before, so hard to answer your question!

Comment 2

[Chris Kitching [:ckitching]]

I've not investigated in detail what happened - but I do know that when I went just now to deal with Richard's latest comments over in the giant favicon bug all the changes to LoadFaviconTask and some chunks from TwoLinePageRow had bitrotted. Plus, there's now a bunch of extra references to the LoadFaviconTask (Same name, different definition) which was used in TwoLinePageRow.

My point is, it looks like there's been more changes made that could be responsible for this than just the ones you've mentioned. Bug 905865 landed quite a while ago - surely we'd have noticed if that was at fault?
As for Bug 922116 - the change there is so simplistic, it could only be me who manages to screw it up so well that it causes this bug :P.
Perhaps worth investigating those other changes have happened in the area recently. I would do so myself, but alas, lectures exist.

Comment 3

[Richard Newman [:rnewman]]

These touch TwoLinePageRow since Bug 905865:

changeset:   150187:5448a5ffcccf
user:        Lauren Ko <[e-mail]>
date:        Thu Oct 03 17:13:42 2013 -0700
summary:     Bug 919301 - Stop showing "switch to tab" from normal browsing when in private browsing. r=mleibovic

changeset:   148078:69347665c4f1
user:        Sriram Ramasubramanian <[e-mail]>
date:        Thu Sep 19 11:29:34 2013 -0400
summary:     Bug 917848: Use a static class for LoadFaviconTask inside TwoLinePageRow. [r=lucasr]

changeset:   147710:93185fb06908
user:        Lucas Rocha <[e-mail]>
date:        Wed Sep 18 12:58:58 2013 -0400
summary:     Bug 905685 - Clear favicon until the new image finishes loading in TwoLinePageRow (r=sriram)

Comment 4

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #1)
> Depends what you call "slow". Favicon loads keep up with a page-by-page
> inertial scroll when I swipe. Definitely nowhere near instantaneous, but of
> course instantaneous would come with its own cost.
> 
> I don't recall what the behavior was like before, so hard to answer your
> question!

Just look at the awesomescreen lists on beta/release. The favicons are always visible as you scroll.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #4)
> (In reply to Richard Newman [:rnewman] from comment #1)
> > Depends what you call "slow". Favicon loads keep up with a page-by-page
> > inertial scroll when I swipe. Definitely nowhere near instantaneous, but of
> > course instantaneous would come with its own cost.
> > 
> > I don't recall what the behavior was like before, so hard to answer your
> > question!
> 
> Just look at the awesomescreen lists on beta/release. The favicons are
> always visible as you scroll.

Isn't it just a result of not pre-loading all favicons in memory anymore when the list is first displayed? This approach was fragile in many ways (see bug 905685). Sriram is working on a patch to use Smoothie on all listviews in about:home (see bug 919777). Smoothie supports incremental pre-loading of offscreen items, which will definitely help this issue.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Agreed. This seems like a dupe of bug 919777.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

We want to focus this bug for favicon loading on Fx26. Bug 919777 will be focused on Fx27

Comment 8

[:Margaret Leibovic]

Sriram volunteered to work on this - thanks!

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Use FaviconsLoader

This uses favicons loader for first 26 urls.

Comment 10

[Sriram Ramasubramanian [:sriram]]

This patch is done only for MostRecentPage. Based on if we want to take it, I can post patches for other lists too.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 816115 [details] [diff] [review]
Use FaviconsLoader

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

Won't this cause FaviconsLoader and all individual LoadFaviconTasks for each TwoLinePageRow run concurrently when you first load the list?

Comment 12

[Chris Kitching [:ckitching]]

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Comment on attachment 816115 [details] [diff] [review]
> Use FaviconsLoader
> 
> Review of attachment 816115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Won't this cause FaviconsLoader and all individual LoadFaviconTasks for each
> TwoLinePageRow run concurrently when you first load the list?

Looks like it will.

Perhaps a good approach would be to have FaviconsLoader just pre-fill the in-memory cache? Perhaps implement it as a "Put all these favicons in the memcache now" method in Favicons?
Just a thought.

Another thought - does FaviconsLoader use the combined_with_favicons view? You'll get much better performance if you make it query the favicons table directly. (Very, very much better performance).

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 816115 [details] [diff] [review]
Use FaviconsLoader

Clearing feedback flag. Patch still needs some work I guess.

Comment 14

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #11)
> Comment on attachment 816115 [details] [diff] [review]
> Use FaviconsLoader
> 
> Review of attachment 816115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Won't this cause FaviconsLoader and all individual LoadFaviconTasks for each
> TwoLinePageRow run concurrently when you first load the list?

By that time, the list loads, the favicons are already in memory. No?

Comment 15

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> (In reply to Lucas Rocha (:lucasr) from comment #11)
> > Comment on attachment 816115 [details] [diff] [review]
> > Use FaviconsLoader
> > 
> > Review of attachment 816115 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Won't this cause FaviconsLoader and all individual LoadFaviconTasks for each
> > TwoLinePageRow run concurrently when you first load the list?
> 
> By that time, the list loads, the favicons are already in memory. No?

Well, not necessarily. What will happen is that, on startup, we'll have 4-5 async tasks to load the favicon for each visible row and, at the same time, the FaviconsLoader loading a bunch of favicons into memory.

My main concern here is the possibility of performance regressions with all these things happening at the same time. Ideally, we should only run the FaviconsLoader on startup, with no concurrent async tasks.

Comment 16

[Richard Newman [:rnewman]]

WONTFIX 'cos this is addressed in 27, and we don't want to destabilize beta.