Closed Bug 460301 Opened 12 years ago Closed 12 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
mozilla1.9.1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Details

Attachments

(2 files)

77.74 KB, image/png
Details
59.08 KB, application/vnd.openxmlformats-officedocument.presentationml.presentation
Details
If we are going to overwrite an existing favicon because it has expired, we should not insert the same data that we had before.
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.
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...
(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
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?
(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.
Attached image 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.
in case anyone ever needs to update this...
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
Closed: 12 years ago
Flags: blocking1.9.1?
Resolution: --- → WONTFIX
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.