GNOME Bugzilla – Bug 721895
Remove GtkIconCache
Last modified: 2014-06-28 05:42:04 UTC
The icon cache on my system is over 100M. Given how most apps don't use that many non-symbolic icons anymore, I don't think we need fast mmap-able icon data on disk.
Created attachment 265881 [details] [review] Remove GtkIconCache
I'm not sure the icon data is very important anymore. However, this deletes all of the cache, not just the pixel data. This means every app will readdir all the theme dirs, which is very slow. I don't think that is a very good idea.
Created attachment 265959 [details] [review] updateiconcache: Don't include image data by default anymore Since large images are in the icon cache, and apps don't tend to use that many icons anymore, simply don't include image data and instead make apps load files from disk. Additionally, since they're stored in GdkPixbuf data, that means that we have to first convert them either to a cairo_surface_t, which requires converting pixel data to be premulitplied, or an OpenGL texture, which requires a whole GPU upload anyway. So, even with the icon cache, the goal of icons through zero-copy, mmap()'d data from disk just isn't doable with the icon cache format we have. The icon cache on my disk is nearing 100MB, since we include a bunch of high-resolution application icons, that I doubt would be used by apps at all. Removing this inefficient pixel data makes memory usage for all applications go down, with no speed loss. The icon cache also, however, has an index of what icons are in each folder, which prevents a readdir() and allows GTK+ to know what icon is where without having to do a bunch of stat(); calls. Keeping this data is good for GTK+, so we should still keep the index. It doesn't make sense to remove any code for mapping pixel data from the icon cache. There's a plan in the works to have a symbolic icon cache that does pixel math on 16x16 icons to prevent slow SVG rendering. 16x16 pixels are fairly small, and such images are flat colors, which should compress easily, so the icon cache would be worthwhile here. So let's keep the code around in preparation for that case.
I'd like to see some actual measurements of how this affects memory consumption and io.
I wouldn't say that this affects "memory use", as it is just (shared) mapped memory, i.e. it just "looks like" it uses memory, but it can be immediately thrown out if memory is needed elsewhere. Otoh, it *does* use disk space, and (on 32bit arches) vm space, while at the same time not being really useful. In fact, due to us not using the pixmap format long term anyway the major difference is that it avoids doing the png decompression at the cost of having to read more bytes from the disk. With the current CPU speeds I'd guestimate that the decompression is faster than the disk reads, so this may be a net negative. The two useful options we have are imho: a) Completely remove image data from cache, or b) store the original compressed files in cache (to avoid seeks on read).
Also, even if we store the compressed files in the cache we don't necessarily have to mmap the whole file. We could mmap the initial part and then use the cache fd + offset with pread() to stream the pixel data to gdk-pixbuf. That way we avoid using vm space, yet avoid the inode seeks. Is avoiding the inode seek important? I dunno. If you read many small icon files it is. For large ones, or ones that are rarely used, not so much. I guess it would be nice to have some actual data.
Review of attachment 265959 [details] [review]: A refreshingly simple patch. But if we make this change, we should probably replace the --index-only commandline option with a --include-image-data option that does the opposite - unless we decide to strip out the image data support altogether
Here's the disk size comparison: Before: -rw-r--r--. 1 root root 70704780 Oct 16 16:43 /usr/share/icons/gnome/icon-theme.cache -rw-r--r--. 1 root root 24754884 Jan 9 06:18 /usr/share/icons/hicolor/icon-theme.cache -rw-r--r--. 1 root root 120581040 Jan 6 16:55 /usr/share/icons/HighContrast/icon-theme.cache After: -rw-r--r--. 1 root root 115532 Jan 13 21:06 /usr/share/icons/gnome/icon-theme.cache -rw-r--r--. 1 root root 17604 Jan 13 21:06 /usr/share/icons/hicolor/icon-theme.cache -rw-r--r--. 1 root root 44384 Jan 13 21:06 /usr/share/icons/HighContrast/icon-theme.cache
Also note that we have the --enable-gtk2-dependency configure flag, so whatever changes we make to updateiconcache in gtk3 will also need to go into gtk2.
Created attachment 266613 [details] strace -e open gedit | grep share/icons/gnome Here is what I see when I start gedit with an index-only icon cache: We're opening 98 files in /usr/share/icons/gnome 14 of these are svgs, those would have been opened with any icon cache 81 are pngs whats interesting is that we seem to be opening every file 3 times. First, with O_NOATIME, which fails with EPERM. open(2) says: EPERM The O_NOATIME flag was specified, but the effective user ID of the caller did not match the owner of the file and the caller was not privileged (CAP_FOWNER). Is this something we should avoid, or is it harmless ? That still leaves the other two times the file is opened - seems like one too many ?
For comparison: strace -e open gimp | grep share/icons/gnome -> 54
glocalfileinfo.c is where the noatime, and the double-open comes from: content_type = g_content_type_guess (basename, NULL, 0, &result_uncertain); #ifndef G_OS_WIN32 if (!fast && result_uncertain && path != NULL) { guchar sniff_buffer[4096]; gsize sniff_length; int fd; sniff_length = _g_unix_content_type_get_sniff_len (); if (sniff_length > 4096) sniff_length = 4096; #ifdef O_NOATIME fd = g_open (path, O_RDONLY | O_NOATIME, 0); if (fd < 0 && errno == EPERM) #endif fd = g_open (path, O_RDONLY, 0); So, why is the 'image/png' and uncertain guess for a filename ending in .png ?! That is because somebody thought it was cool to add a image/x-apple-ios-png type with the same glob to shared-mime-info :-(
Some pointers: http://cgit.freedesktop.org/xdg/shared-mime-info/commit/?id=afbdfb40a3ff8c255e5d3dab8840cc478a007d45 http://iphonedevelopment.blogspot.co.uk/2008/10/iphone-optimized-pngs.html
The other question is "why are we doing a content type lookup when gdk-pixbuf will do the detection for us?" The answer is "because we need to test if it's an SVG or not". Except, we already know... Patch incoming soon.
Created attachment 266614 [details] [review] gtkicontheme: Don't query CONTENT_TYPE to determine if something is an SVG We already know based on the suffix of the filename.
I'm a little worried that there's many places that create icon infos all over the place. You've fixed one to set is_svg. What about all the others ?
Which others? I can only count three places where we assign icon_file -- one in choose_icon, and the other two in theme_lookup_icon. theme_lookup_icon is only called by choose_icon, and after both of those, we "goto out;", so we'll hit that path.
Review of attachment 266614 [details] [review]: ok then
Comment on attachment 266614 [details] [review] gtkicontheme: Don't query CONTENT_TYPE to determine if something is an SVG Attachment 266614 [details] pushed as f929a61 - gtkicontheme: Don't query CONTENT_TYPE to determine if something is an SVG
reverted for now
It seemed to cause the wrong icon size to be loaded for SVGs. I have no idea why.
Jasper: Any idea which particular icons it was? Should be easy to add some code to the sniffer and report whatever filenames were sniffed as svg, but did not have a .svg suffix.
App icons in the gnome-shell overview, from my recollection.
Maybe these: /usr/share/icons/hicolor/scalable/apps/k3b.svgz /usr/share/icons/hicolor/scalable/apps/phonon-gstreamer.svgz /usr/share/icons/hicolor/scalable/apps/knetattach.svgz /usr/share/icons/hicolor/scalable/apps/konqueror.svgz
Or maybe unthemed icons. Transmageddon uses /usr/share/pixmaps/transmageddon.svg which will not be hit by the patch.
(i.e. is_svg will not be set)
Lets try this again; I couldn't see any problem with the patch, on my system.
Created attachment 278609 [details] [review] icon theme: Fix crash for builtin icons This pushes the initialization of is_svg to an earlier point when the GtkIconInfo is created. This is needed because an icon info doesn't necessarily always have a filename that we can later extract the suffix from. For instance, builtin icons have NULL filename.
Comment on attachment 278609 [details] [review] icon theme: Fix crash for builtin icons Attachment 278609 [details] pushed as 3574a80 - icon theme: Fix crash for builtin icons
I think we have this under control now.