Bug 82346 – [GTK] Purge unused favicons from IconDatabase after 30 days

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[GTK] Purge unused favicons from IconDatabase after 30 days

[Sergio Villar Senin]

Created attachment 134081 [details]
Patch

[Martin Robinson]

Comment on attachment 134081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134081&action=review

> Source/WebCore/loader/icon/IconDatabase.cpp:85
> +#if PLATFORM(GTK)
> +// We are not interested on icons older than 30 days, so we'll delete
> +// them if they have not been used after that time even if they are
> +// not explicitely released.
> +static const int notUsedIconExpirationTime = 60*60*24*30;
> +#endif
> +

Why not simply use an #ifdef for the iconExpirationTime above?

> Source/WebCore/loader/icon/IconDatabase.cpp:1729
> +    GOwnPtr<char> deleteQuery(g_strdup_printf("DELETE FROM PageURL WHERE iconID IN (SELECT iconID FROM IconInfo WHERE stamp <= '%f');", currentTime() - notUsedIconExpirationTime));

If this line has to stay you should use String::format so that this code is platform-independent.

[Sergio Villar Senin]

(In reply to comment #2)
> (From update of attachment 134081 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134081&action=review
> 
> > Source/WebCore/loader/icon/IconDatabase.cpp:85
> > +#if PLATFORM(GTK)
> > +// We are not interested on icons older than 30 days, so we'll delete
> > +// them if they have not been used after that time even if they are
> > +// not explicitely released.
> > +static const int notUsedIconExpirationTime = 60*60*24*30;
> > +#endif
> > +
> 
> Why not simply use an #ifdef for the iconExpirationTime above?

Because that's a different thing. iconExpirationTime is used to "refresh" icons after 4 days. It isn't used to purge them but just to ensure that the database keeps always a quite recent copy of the icon. So this patch does not remove that feature and thus we still need it.

> > Source/WebCore/loader/icon/IconDatabase.cpp:1729
> > +    GOwnPtr<char> deleteQuery(g_strdup_printf("DELETE FROM PageURL WHERE iconID IN (SELECT iconID FROM IconInfo WHERE stamp <= '%f');", currentTime() - notUsedIconExpirationTime));
> 
> If this line has to stay you should use String::format so that this code is platform-independent.

Well we're only pruning the database in the GTK port so far, but I agree that using platform-independent code is better anyway.

[Sergio Villar Senin]

Created attachment 134095 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #4)
> Created an attachment (id=134095) [details]
> Patch

The previous version of the patch was not considering the case when stamp==0 which means that the icon is in the database but the actual icon data was not written to disk yet.

[Sergio Villar Senin]

As I told Xan on IRC I think that it'd be better to use a different transaction to this operation. That way it wouldn't interfere with the previous pageURL deletions. Note that the pruning code is executed just once per execution at most so the performance impact of a single transaction is really negligible.

Some other thing to consider would be to enable this for all the other platforms. I think all of them would be interested. Brady any thoughts on this?

[Xan Lopez]

Comment on attachment 134095 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134095&action=review

This looks OK to me. If you want to move the 30day pruning to another transaction feel free, but I don't think it's mandatory.

> Source/WebCore/loader/icon/IconDatabase.cpp:77
> +// them if they have not been used after that time even if they are

Nitpick, but I think this sentence is a bit confusing. I'd just write "We are not interested in icons that have been unused for more than 30 days, delete them even if they have not been explicitly released".

> Source/WebCore/loader/icon/IconDatabase.cpp:1724
> +    // Delete icons not used in the last 30 days

I'd add "even if they are not retained", and missing dot at the end.

> Source/WebCore/loader/icon/IconDatabase.cpp:1725
> +    String deleteQuery = String::format("DELETE FROM PageURL WHERE iconID IN (SELECT iconID FROM IconInfo WHERE stamp <= %.0f AND stamp > 0);", floor(currentTime() - notUsedIconExpirationTime));

So this just deletes the PageURL stuff and the actual icon data will be pruned later in this same method. I think it would be worth making that clear, since the comment says "Delete icons".

[Xan Lopez]

Comment on attachment 134095 [details]
Patch

Sergio has mentioned he wants to do this in a different way now, so removing r+.

[Sergio Villar Senin]

Created attachment 134260 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 134260 [details]
Patch

This looks good to me. I wonder if it doesn't make sense to have this change enabled for everyone though. Do you mind asking the "maintainers" of the icon database whether they'd be interested in it? suggest-reviewers gives me:


Adam Roben
Alexey Proskuryakov
Anders Carlsson
David Levin
Geoffrey Garen
George Staikos
Mark Rowe
Steve Block

[Sergio Villar Senin]

(In reply to comment #10)
> (From update of attachment 134260 [details])
> This looks good to me. I wonder if it doesn't make sense to have this change enabled for everyone though. Do you mind asking the "maintainers" of the icon database whether they'd be interested in it? suggest-reviewers gives me:

Brady do you think this might be interesting also for the rest of the platforms?

[Brady Eidson]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 134260 [details] [details])
> > This looks good to me. I wonder if it doesn't make sense to have this change enabled for everyone though. Do you mind asking the "maintainers" of the icon database whether they'd be interested in it? suggest-reviewers gives me:
> 
> Brady do you think this might be interesting also for the rest of the platforms?

I haven't looked at the patch yet.  "Purge unused icons after 30 days..." of what?  30 days since they were set?  

In general this bug report proposes a conclusion "Purge unused icons after 30 days" with no explanation as to why this is desirable.

Without further exploration, this seems quite undesirable to us and we definitely don't want this.

[Sergio Villar Senin]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 134260 [details] [details] [details])
> > > This looks good to me. I wonder if it doesn't make sense to have this change enabled for everyone though. Do you mind asking the "maintainers" of the icon database whether they'd be interested in it? suggest-reviewers gives me:
> > 
> > Brady do you think this might be interesting also for the rest of the platforms?
> 
> I haven't looked at the patch yet.  "Purge unused icons after 30 days..." of what?  30 days since they were set?  
> 
> In general this bug report proposes a conclusion "Purge unused icons after 30 days" with no explanation as to why this is desirable.
> 
> Without further exploration, this seems quite undesirable to us and we definitely don't want this.

Yeah sure, I should have added some extra information in the bug report. Everything is in the patch but yeah I should have mentioned the rationale here.

So basically this patch proposes to purge from the database all the icons that haven't been used during the last 30 days. This might be interesting because otherwise the icon database might grow forever if applications decide to retain every icon the database stores. It might be argued that the responsibility belongs to the apps but in this case I think most of them are interested in keeping the database under control.

[Brady Eidson]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > (From update of attachment 134260 [details] [details] [details] [details])
> > > > This looks good to me. I wonder if it doesn't make sense to have this change enabled for everyone though. Do you mind asking the "maintainers" of the icon database whether they'd be interested in it? suggest-reviewers gives me:
> > > 
> > > Brady do you think this might be interesting also for the rest of the platforms?
> > 
> > I haven't looked at the patch yet.  "Purge unused icons after 30 days..." of what?  30 days since they were set?  
> > 
> > In general this bug report proposes a conclusion "Purge unused icons after 30 days" with no explanation as to why this is desirable.
> > 
> > Without further exploration, this seems quite undesirable to us and we definitely don't want this.
> 
> Yeah sure, I should have added some extra information in the bug report. Everything is in the patch but yeah I should have mentioned the rationale here.
> 
> So basically this patch proposes to purge from the database all the icons that haven't been used during the last 30 days.

"Used" means accessed at all, even read?

How does it enforce this?  Does it add information to the schema and writes to the database just for read-only access?

If so, then this is a performance hit for platforms that don't want this feature.  And a performance hit of the worst kind - additional I/O.

> This might be interesting because otherwise the icon database might grow forever if applications decide to retain every icon the database stores. 

If an application decides to retain every icon in the database, then they are saying they don't want any icon to go away.  If the application actually does want an icon to go away even when they are retaining it, they are misusing the API.

>It might be argued that the responsibility belongs to the apps but in this case I think most of them are interested in keeping the database under control.

An application that is interested in keeping the database under control should only retain icons it cares about.

In Safari's case, for example, it retains icons associated with Bookmark entries as well as history entries.  If the icon was there at one time, it expects the icon to be there in the future even if the user doesn't look at that particular bookmark or history entry for 30 days.

[Brady Eidson]

Okay I see the patch doesn't affect non-gtk ports.

I still know it's not something everybody is interesting in (since Apple doesn't want it), so please keep the ifdef's in place.

[Sergio Villar Senin]

(In reply to comment #14)
> > So basically this patch proposes to purge from the database all the icons that haven't been used during the last 30 days.
> 
> "Used" means accessed at all, even read?
> 
> How does it enforce this?  Does it add information to the schema and writes to the database just for read-only access?
> If so, then this is a performance hit for platforms that don't want this feature.  And a performance hit of the worst kind - additional I/O.

I think in this case it's better if you take a look to the patch instead of writing preconceived judgments (it's really small, it won't take you too much to review it). I'm using the IconInfo.stamp information. As seen in the patch, I know that the stamp is only modified when the icon is downloaded from the server. The good thing is that the icon database refreshes icons every 4 days if they're used, so we can use that information to infer that the icons were not used during the last X days (otherwise they'd have been downloaded from the server again).

> > This might be interesting because otherwise the icon database might grow forever if applications decide to retain every icon the database stores. 
> 
> If an application decides to retain every icon in the database, then they are saying they don't want any icon to go away.  If the application actually does want an icon to go away even when they are retaining it, they are misusing the API.
> 
> >It might be argued that the responsibility belongs to the apps but in this case I think most of them are interested in keeping the database under control.
> 
> An application that is interested in keeping the database under control should only retain icons it cares about.

That's the point of this patch. Aren't 99% of applications interested in having such a control over the database?
 
> In Safari's case, for example, it retains icons associated with Bookmark entries as well as history entries.  If the icon was there at one time, it expects the icon to be there in the future even if the user doesn't look at that particular bookmark or history entry for 30 days.

Fair enough.

[Brady Eidson]

(In reply to comment #16)
> (In reply to comment #14)
> > > So basically this patch proposes to purge from the database all the icons that haven't been used during the last 30 days.
> > 
> > "Used" means accessed at all, even read?
> > 
> > How does it enforce this?  Does it add information to the schema and writes to the database just for read-only access?
> > If so, then this is a performance hit for platforms that don't want this feature.  And a performance hit of the worst kind - additional I/O.
> 
> I think in this case it's better if you take a look to the patch instead of writing preconceived judgments (it's really small, it won't take you too much to review it). 

If you look at comment 15 - 2 days before you wrote this - you'll see I already did that.

> > > This might be interesting because otherwise the icon database might grow forever if applications decide to retain every icon the database stores. 
> > 
> > If an application decides to retain every icon in the database, then they are saying they don't want any icon to go away.  If the application actually does want an icon to go away even when they are retaining it, they are misusing the API.
> > 
> > >It might be argued that the responsibility belongs to the apps but in this case I think most of them are interested in keeping the database under control.
> > 
> > An application that is interested in keeping the database under control should only retain icons it cares about.
> 
> That's the point of this patch. Aren't 99% of applications interested in having such a control over the database?

I'm confused about this statement.

My assertion was that applications *already* have control of the icons that the database preserves.  By retaining a URL, that icon is not pruned from the database.

This bug and this patch purports to add an addition *implicit* mechanism of keeping icons around for 30 days.  That is *not* something the app would control.

[Sergio Villar Senin]

(In reply to comment #17)
> (In reply to comment #16)
> > I think in this case it's better if you take a look to the patch instead of writing preconceived judgments (it's really small, it won't take you too much to review it). 
> 
> If you look at comment 15 - 2 days before you wrote this - you'll see I already did that.

I was talking about your comment about modifying the schema, that the patch does not do. Anyway it does not make much sense to discuss about that.

> > That's the point of this patch. Aren't 99% of applications interested in having such a control over the database?
> 
> I'm confused about this statement.
> 
> My assertion was that applications *already* have control of the icons that the database preserves.  By retaining a URL, that icon is not pruned from the database.

Yeah I understand your confusion, it was pretty bad written by my side. I know apps have all the control, what I wanted to say is that a high percentage of the applications want all the icons to be retained by default while keeping the database under control.

The point here is that in the GTK port we do not expose all the retain/release logic, but instead retain all the icons by default. That's why we need such a mechanism. But I agree that your approach of exposing all the retain/release do not require such an implicit database control mechanism because you already have to take care of all the icons on each application.

[Gustavo Noronha (kov)]

And the apple approach may make sense for us too, I guess. We could expire any icons the application didn't specifically request retaining, for instance?

[Sergio Villar Senin]

(In reply to comment #19)
> And the apple approach may make sense for us too, I guess. We could expire any icons the application didn't specifically request retaining, for instance?

That means opening a debate that I thought it was closed. When we implemented the icondatabase API in bug 56200 we decided to retain every icon by default because we thought that exposing the retain/release API was a mistake as most applications would want to retain icons by default.

So either we retain by default and purge, or we expose the retain/release API.

[Eric Seidel (no email)]

I could rubber-stamp this Gtk-only change, but it seems martin or xan or gns are better at that.

[Gustavo Noronha (kov)]

Comment on attachment 134260 [details]
Patch

In that case, let's do it.

[Sergio Villar Senin]

Comment on attachment 134260 [details]
Patch

Clearing flags on attachment: 134260

Committed r126551: <http://trac.webkit.org/changeset/126551>

[Sergio Villar Senin]

All reviewed patches have been landed.  Closing bug.