Do not write to the database if the favicon has not changed

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
10 years ago
10 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla1.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

77.74 KB, image/png
Details
59.08 KB, application/vnd.openxmlformats-officedocument.presentationml.presentation
Details
(Assignee)

Description

10 years ago
If we are going to overwrite an existing favicon because it has expired, we should not insert the same data that we had before.

Comment 1

10 years ago
a check on length of data and favicon url could be enough i guess, since checking on the data would do the compare with memcmp that i guess is slower than checking length.
however we should generally save less than 1KB favicons (we have code to optimize their size to that value) and we don't save favicons bigger than 10KB, so maybe also direct compare may not be too slow.
(Assignee)

Comment 2

10 years ago
I just want to check - we do not remove favicons proactively if they have expired, do we?  I haven't found code that does that at least...

Comment 3

10 years ago
(In reply to comment #2)
> I just want to check - we do not remove favicons proactively if they have
> expired, do we?  I haven't found code that does that at least...

no, we are not removing them on expiration
(Assignee)

Comment 4

10 years ago
Requesting blocking on this.  We will end up writing the favicon out to a site once every day if the user visits the site daily, or every time they visit the site if they visit it over longer intervals.  This won't show up in our Tp benchmark, but would show up for users.
Flags: blocking1.9.1?
(Assignee)

Comment 5

10 years ago
(In reply to comment #1)
> a check on length of data and favicon url could be enough i guess, since
> checking on the data would do the compare with memcmp that i guess is slower
> than checking length.
> however we should generally save less than 1KB favicons (we have code to
> optimize their size to that value) and we don't save favicons bigger than 10KB,
> so maybe also direct compare may not be too slow.
We can always not compare them if the length isn't the same, and then compare if it is.
(Assignee)

Comment 6

10 years ago
Created attachment 347882 [details]
state diagram

I was going crazy trying to keep all the states in my head, so I made a state diagram.  Yay for code documentation.
(Assignee)

Comment 7

10 years ago
Created attachment 347883 [details]
state diagram source file

in case anyone ever needs to update this...
(Assignee)

Comment 8

10 years ago
It turns out that we cannot really win anything here.  At best, we'll save 10k in writes, but in most cases, we'll save 1k in writes for the favicon, but we still have to touch that database page since we have to update the expiration time.  We also have to update the favicon id for the page (which will be cheap since we are doing it in the temp table), and possibly update any bookmark redirects.  Essentially, we'll get marginal gains (at best) for a large increase in code complexity.  As a result, this bug is WONTFIX.  Our efforts will be better spent coming up with a smarter expiration strategy (bug 460300).
No longer blocks: 442967
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: blocking1.9.1?
Resolution: --- → WONTFIX

Comment 9

10 years ago
i was thinking about a trigger on moz_places that when visit_count is updated updates the favicon expiration if favicon_id is defined....
not sure that would help though
You need to log in before you can comment on or make changes to this bug.