GNOME Bugzilla – Bug 692934
Defer cairo surface creation for image URLs in CSS themes
Last modified: 2013-02-04 12:41:28 UTC
Created attachment 234900 [details] [review] cssimageurl: Defer cairo surface creation I've been working on memory use in a number of Ubuntu desktop applications recently. One of the things I noticed when running valgrind over "update-notifier" (an application that occasionally pops up to notify you of things like "you have updates available" or "you need to reboot if you want to actually use the new kernel you just installed") was that its top consumer of heap memory by some margin - a couple of hundred KB - was images in CSS themes. This struck me as odd, since it allocates all this memory even if it never needs to pop up and display any UI at all! Notifiers are an extreme case, but even in more normal applications you might well never need to display all of the images in a complex theme. Looking at gtkcssimageurl.c, it's quite simple to defer creating the cairo surface until the first time the image is drawn. In my not-desperately-scientific testing, this trimmed 6MB of memory off an Ubuntu desktop image running on the Nexus 7 (after a fresh reboot and waiting a few minutes for everything to settle). It would probably be technically possible to avoid creating the pixbufs too, which is the next largest consumer of heap memory after this change; or even to avoid reading the contents of the image URLs until they're needed. However, I'm not sure what needs to be done with the error paths here. If anyone would care to advise on that then I could prepare a follow-up patch, perhaps in a separate bug.
As a baseline, on the same Nexus 7 system, this drops the RSS of a trivial GTK+ program (gtk_init; gtk_main) from 6568KB to 6344KB. (Plenty of that is code size, of course.)
As a first step, you could create the theme as a resource. This is a kind of undocumented feature of GTK that we use for Adwaita. That way pixbufs will be mmap()ed from disk and don't need to be decoded. The relevant links for that are http://git.gnome.org/browse/gnome-themes-standard/commit/?id=8302a8f04e0fd363ec807249237c96fdc55a63c5 which introduced support for that (there might be important fixes later in the git log) and http://git.gnome.org/browse/gtk+/tree/gtk/gtkcssprovider.c?id=eb4667b6e1672043a9d9ff75a959e82b133618a7#n2809 which is the code in GTK that loads the resource. That support is available in 3.6, not sure if we had it in 3.4 already.
Making the images resources might possibly help elsewhere, and I agree we probably should - but as far as I can see that doesn't stop gtkcssimageurl creating the cairo surface, which it can avoid doing up-front even for resources.
As a second step, I think splitting GtkCssImageURL into a GtkCssImageUrl and GtkCssImageSurface is not a bad idea if you want to save memory. The image->compute() vfunc should probably do the on-demand creation of the GtkCssImageSurface and store a weak reference to it in the GtkCssImageUrl. As compute() is called whenever we create an actual value for rendering from the style context, we can then load and unload the data on-demand. I'm not sure if I would store a GdkPixbuf or a GFile in the GtkCssImageUrl. Storing a GFile would make it really light for the case where it's real files, storing a GdkPixbuf would only benefit in the resources case. I'm leaning towards storing the GFile.
(In reply to comment #0) > Created an attachment (id=234900) [details] [review] > cssimageurl: Defer cairo surface creation > I don't like this patch because it creates a pixbuf _and_ a surface. So in the normal case where files and not resources are loaded, we end up with the decoded image data twice: once in the pixbuf and once in the surface. I suspect the savings you measure come mostly from the fact that GdkPixbuf uses 3bpp and cairo surfaces use 4bpp, so you save 25% for every not-yet loaded image.
You may well be right about more general refactoring. But I am thoroughly confused by comment #5: my patch does no such thing that wasn't there before. Right now, gtkcssimageurl creates both a pixbuf and a surface, clearing the pixbuf after the surface is created. All I did was move the surface creation later; it still clears the pixbuf after the surface is created.
Oh, I didn't see that. So there's only ever a surface or a pixbuf existing. That would indeed solve the case of mmaped pixbuf (the resources case) but would otherwise only buy at most a 25% improvement. And I think the code looks a lot more ugly as a result. I'll see about implementing the surface approach to see how hard it is. I like that approach because it moves the code in a direction where it solves a bunch of other issues I still have with it. Do you have a way to measure the improvements, so that I can easily see if I got the benefits right for the Android use case?
I've pushed a bunch of patches to master that implement this behavior. According to me running gtk_init() through valgrind --tool=memcheck, they allocate 6B less of memory. So I think they fix the problem you were reporting. I'd still appreciate if you tested that they indeed fix things for you and I'm not just making things up. The commits are tagged with this bug# should you feel the desire to backport them somewhere. The following link should give you a diff with all commits rolled into one: http://git.gnome.org/browse/gtk+/diff/?id=e6b3cbe7d2c8ad23284bcf4a17f350043e3afb54&id2=5607a2125fa712f99abf21901d704f1831d7cea4
Looks fantastic, thanks. A bare {gtk_init; gtk_main} uses a bit less memory, valgrind of update-notifier is very much cleaner (largest allocation is now 32768 bytes, vs. earlier 107420 bytes; total used according to valgrind is 937789 bytes, vs. earlier 1169694 bytes), and whole-system memory use appears to have dropped by at least 19MB.