After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 689081 - No caching of icons
No caching of icons
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
3.5.x
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2012-11-26 12:56 UTC by Alexander Larsson
Modified: 2012-11-30 10:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Cache GtkIconInfo (6.72 KB, patch)
2012-11-26 12:56 UTC, Alexander Larsson
none Details | Review
Reuse rendered symbolic icons (13.21 KB, patch)
2012-11-26 12:57 UTC, Alexander Larsson
none Details | Review
Cache GtkIconInfo (7.08 KB, patch)
2012-11-27 08:31 UTC, Alexander Larsson
none Details | Review
Reuse rendered symbolic icons (13.21 KB, patch)
2012-11-27 08:31 UTC, Alexander Larsson
none Details | Review
Cache GtkIconInfo (7.08 KB, patch)
2012-11-29 22:21 UTC, Alexander Larsson
none Details | Review
Cache GtkIconInfo (6.90 KB, patch)
2012-11-29 22:31 UTC, Alexander Larsson
none Details | Review
Cache GtkIconInfo (11.83 KB, patch)
2012-11-30 10:33 UTC, Alexander Larsson
committed Details | Review
Reuse rendered symbolic icons (15.81 KB, patch)
2012-11-30 10:33 UTC, Alexander Larsson
committed Details | Review

Description Alexander Larsson 2012-11-26 12:56:43 UTC
We keep re-reading icons from the icon theme, even if we looked up them before. This is pretty bad in general, but especially so if icons are used in a list like in bug 687911. Its especially bad for symbolic icon SVGs, as these are not in the icon theme cache and re-parsed every time.
Comment 1 Alexander Larsson 2012-11-26 12:56:59 UTC
Created attachment 229899 [details] [review]
Cache GtkIconInfo

In order to avoid loading and keeping around the same icon multiple times
we keep a cache of all outstanding GtkIconInfo objects for a given theme.
We also keep alive the 32 last recently looked up infos, as most code
don't keep the IconInfo object alive.
Comment 2 Alexander Larsson 2012-11-26 12:57:01 UTC
Created attachment 229900 [details] [review]
Reuse rendered symbolic icons

With the previous commit all loads of the same icon will share a single
GtkIconInfo, which typicallty means the pixbuf is shared via Info->pixbuf.

However, atm we don't share symbolic icons, which causes these to be re-read
and re-parsed every time. This is especially bad if the icon is used many times
in some form of list. So, we cache the last pixbuf and reuse it as long as the
colors are the same.
Comment 3 Matthias Clasen 2012-11-26 21:54:50 UTC
It feels like eventually we should redo GtkIconTheme more thoroughly.
Maybe that can wait until Benjamin has his css icon ideas worked out.
Comment 4 Alexander Larsson 2012-11-27 07:47:14 UTC
Its clearly grown out of scope. But, otoh, GtkIconInfo was explicitly designed in order to facilitate cacheing, thats what NautilusIconInfo was used for that got moved to Gtk+ to allow centralized caching. The nautilus icon cache keeps refs to the GtkIconInfos in its cache still.

I think this patch is necessary though. As things are right now we're reading svgs from disk for every GtkImage with a symbolic icon on every state change.
Comment 5 Alexander Larsson 2012-11-27 08:31:03 UTC
Created attachment 229971 [details] [review]
Cache GtkIconInfo

In order to avoid loading and keeping around the same icon multiple times
we keep a cache of all outstanding GtkIconInfo objects for a given theme.
We also keep alive the 32 last recently looked up infos, as most code
don't keep the IconInfo object alive.
Comment 6 Alexander Larsson 2012-11-27 08:31:06 UTC
Created attachment 229972 [details] [review]
Reuse rendered symbolic icons

With the previous commit all loads of the same icon will share a single
GtkIconInfo, which typicallty means the pixbuf is shared via Info->pixbuf.

However, atm we don't share symbolic icons, which causes these to be re-read
and re-parsed every time. This is especially bad if the icon is used many times
in some form of list. So, we cache the last pixbuf and reuse it as long as the
colors are the same.
Comment 7 Alexander Larsson 2012-11-27 08:31:50 UTC
New patches, first one had some crashers.
Comment 8 Alexander Larsson 2012-11-29 22:21:40 UTC
Created attachment 230245 [details] [review]
Cache GtkIconInfo

In order to avoid loading and keeping around the same icon multiple times
we keep a cache of all outstanding GtkIconInfo objects for a given theme.
We also keep alive the 32 last recently looked up infos, as most code
don't keep the IconInfo object alive.
Comment 9 Alexander Larsson 2012-11-29 22:22:46 UTC
even with these patches nautilus reloads every symbolic icon you mouse over. We need to cache all the different colored states of the symbolic icons.
Comment 10 Alexander Larsson 2012-11-29 22:31:35 UTC
Created attachment 230247 [details] [review]
Cache GtkIconInfo

In order to avoid loading and keeping around the same icon multiple times
we keep a cache of all outstanding GtkIconInfo objects for a given theme.
We also keep alive the 32 last recently looked up infos, as most code
don't keep the IconInfo object alive.
Comment 11 Alexander Larsson 2012-11-30 10:33:46 UTC
Created attachment 230266 [details] [review]
Cache GtkIconInfo

In order to avoid loading and keeping around the same icon multiple times
we keep a cache of all outstanding GtkIconInfo objects for a given theme.

Additionally we return to the app not the normal pixbuf from the info,
but rather a proxy copy of it sharing the same data, but no extra
reference. This allows us to track when the app is no longer using
the pixbuf, and we can thus ensure that the GtkIconInfo in the cache
stays around for at least as long as the pixbuf is alive.

When the app unrefs the pixbuf we put the Info on a short LRU list
to keep it alive a bit longer, in case the app needs it in a short
while.
Comment 12 Alexander Larsson 2012-11-30 10:33:49 UTC
Created attachment 230267 [details] [review]
Reuse rendered symbolic icons

With the previous commit all loads of the same icon will share a single
GtkIconInfo, which typicallty means the pixbuf is shared via Info->pixbuf.

However, atm we don't share symbolic icons, which causes these to be re-read
and re-parsed every time. This is especially bad if the icon is used many times
in some form of list. So, we cache the pixbufs and reuse them.
Comment 13 Alexander Larsson 2012-11-30 10:36:36 UTC
New approach, we now use a proxy object for the returned pixbuf so that we can track the lifetime of the pixbuf in the app and thus ensure the GtkIconInfo
lives (and is in the cache) while the pixbuf is in use.

Additionally this caches symbolic icons in all the color states, so now nautilus doesn't reaload any symbolic icons unnecessary.
Comment 14 Alexander Larsson 2012-11-30 10:55:35 UTC
Attachment 230266 [details] pushed as 92e904a - Cache GtkIconInfo
Attachment 230267 [details] pushed as cfdc68d - Reuse rendered symbolic icons