1478424 - Eagerly load favicons

Status: NEW

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Dave Townsend [:mossop]]

The current process for loading favicons is something like this:

1. Monitor every new favicon added to the document and add them to a list.
2. If no new favicon has been added in 100ms try to load the preferred one from the list.
3. Forget about the favicons seen up to that point.

This can cause a couple of problems. First if parsing is slow we might throw away import icons in step 3. Second if favicons keep appearing we might not start to load one until the document is complete.

I've been thinking of an alternate method:

1. Detect everytime a new favicon is added to the document.
2. Find the preferred favicon out of all icons in the document.
3. Start loading it, cancelling any existing requests.

In this way we start loading an icon as soon as we see one. If it isn't the correct one we cancel it and start loading the next one when that shows up in the document.

Since icons are normally at the top of the document this would make it more likely that we have the icon before the document load completes. The expense would be increased network traffic.

What do you think to this Marco?

Comment 1

[Marco Bonardo [:mak]]

(In reply to Dave Townsend [:mossop] from comment #0)
> 1. Detect everytime a new favicon is added to the document.
> 2. Find the preferred favicon out of all icons in the document.
> 3. Start loading it, cancelling any existing requests.
> 
> In this way we start loading an icon as soon as we see one. If it isn't the
> correct one we cancel it and start loading the next one when that shows up
> in the document.

How would point 1 and 2 work? I mean, we parse the document, we hit an icon and start loading it... how do we find a preferred icon if we don't have a list of them?

I have no problems with changing the loads behavior, the reason for the current setup though is storing icons, not loading them.
Once a favicon is loaded, how do we decide if it should be stored, if we don't have the whole list?
The only other downside, as you said, is network. Icon loads are already on a low priority queue, though they add to page load time, reason for which I think some other browser loads the favicon after the page.

I think I'm ok with any plan that separates loading from storing, the problem is that the favicon service just stores anything thrown at it (it's mostly a favicon storage service), that means the calling code is responsible to decide what to store, that in general is one favicon (possibly svg or a decently sized one, or an ico) and one rich icon.
We can surely store everything, but it would take a bunch of space.

Comment 2

[Dave Townsend [:mossop]]

(In reply to Marco Bonardo [::mak] from comment #1)
> (In reply to Dave Townsend [:mossop] from comment #0)
> > 1. Detect everytime a new favicon is added to the document.
> > 2. Find the preferred favicon out of all icons in the document.
> > 3. Start loading it, cancelling any existing requests.
> > 
> > In this way we start loading an icon as soon as we see one. If it isn't the
> > correct one we cancel it and start loading the next one when that shows up
> > in the document.
> 
> How would point 1 and 2 work? I mean, we parse the document, we hit an icon
> and start loading it... how do we find a preferred icon if we don't have a
> list of them?
> 
> I have no problems with changing the loads behavior, the reason for the
> current setup though is storing icons, not loading them.
> Once a favicon is loaded, how do we decide if it should be stored, if we
> don't have the whole list?
> The only other downside, as you said, is network. Icon loads are already on
> a low priority queue, though they add to page load time, reason for which I
> think some other browser loads the favicon after the page.
> 
> I think I'm ok with any plan that separates loading from storing, the
> problem is that the favicon service just stores anything thrown at it (it's
> mostly a favicon storage service), that means the calling code is
> responsible to decide what to store, that in general is one favicon
> (possibly svg or a decently sized one, or an ico) and one rich icon.
> We can surely store everything, but it would take a bunch of space.

Good point. I guess I was assuming that we throw away old favicons when we find new ones for a page. Is that not the case currently and what is the reasoning behind that?

We would hit this currently, for example I used to use a gmail setting that set the number of unread mails in the favicon, that meant everytime I received a new mail we'd be storing a new favicon.

We could get around this by using the favicon for the tab eagerly but then only storing it when we have the full set of icons from the page load.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Dave Townsend [:mossop] from comment #2)
> Good point. I guess I was assuming that we throw away old favicons when we
> find new ones for a page. Is that not the case currently and what is the
> reasoning behind that?

We do, this is what happens currently:

1. we get a request to store a new icon X for page Y
2. we check if icon X is in the db, and when it was added
3. if icon X has expired (7 days or depending on cache expiration time set) we remove it
4. if icon X was removed, we store the new version, otherwise do nothing

The problem is that we don't filter icons passed into at all, if the calling code says "please store these 10 favicons for this page", we do that, but in most cases we only need a good favicon for the tab and a good one for Activity Stream.
Even with what we have today (2 icons picked in most cases), the db can be 50MiB or more (We do have telemetry), and we already reduced the max size from 256 to 192 in bug 1475500.

> We would hit this currently, for example I used to use a gmail setting that
> set the number of unread mails in the favicon, that meant everytime I
> received a new mail we'd be storing a new favicon.

That's fine until it's not the common rule. Luckily not many pages use the favicon like that, and that makes it sustainable.

> We could get around this by using the favicon for the tab eagerly but then
> only storing it when we have the full set of icons from the page load.

Yes, that's what I was trying to say in the last paragraph.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

In this way we start loading an icon as soon as we see one.

https://bugzilla.mozilla.org/show_bug.cgi?id=1247843 specifically chose to do the opposite: Put off loading the favicon as much as possible, to allow the page to load faster. As an end-user and website maker, I don't want anything to slow down the page load, including the favicon load.