GNOME Bugzilla – Bug 660968
Simplify the StTextureCache to help facilitate a merge between Mx and St.
Last modified: 2011-11-18 20:44:00 UTC
These should also help memory usage and other things considerably.
Created attachment 198315 [details] [review] StThemeNodeDrawing: Remove useless LoadCornerData Done as part of my quest to merge Mx and St: https://github.com/magcius/mx/tree/st-rebase
Created attachment 198316 [details] [review] StTextureCache: Merge strategies Rather than have five or six structs allocated duplicating data, just keep one and simplify the code considerably. Again, part of my ongoing quest to merge St and Mx.
Review of attachment 198315 [details] [review]: Looks fine I guess.
Comment on attachment 198315 [details] [review] StThemeNodeDrawing: Remove useless LoadCornerData Attachment 198315 [details] pushed as f9b37a2 - StThemeNodeDrawing: Remove useless LoadCornerData
Review of attachment 198316 [details] [review]: I don't understand how simplifying the code helps merging St and Mx? This is a tricky patch and honestly I'd only like to look at this after we've branched. There was a reason I used separate structures here but I'd have to carefully study it to remember why. (And for future reference we should add a comment at the top of the structure defining its lifetime).
(In reply to comment #5) > Review of attachment 198316 [details] [review]: > > I don't understand how simplifying the code helps merging St and Mx? > > This is a tricky patch and honestly I'd only like to look at this after we've > branched. There was a reason I used separate structures here but I'd have to > carefully study it to remember why. (And for future reference we should add a > comment at the top of the structure defining its lifetime). The MxTextureCache is really different than the StTextureCache, so smashing them together is a lot less painful if we can simplify the code and merge some common data structures.
Created attachment 199387 [details] [review] st-texture-cache: Merge strategies Rather than have five or six structs allocated duplicating data, just keep one and simplify the code considerably. Again, part of my ongoing quest to merge St and Mx. Added a lifecycle comment.
Created attachment 199388 [details] [review] st-texture-cache: Use ANSI C-style comments
Created attachment 199389 [details] [review] st-texture-cache: Remove similar duplicated code load_icon_pixbuf_async, load_uri_pixbuf_async, load_thumbnail_async and load_recent_thumbnail_async is the same function four times over. Deduplicate them, as well as cleaning them up so that they don't need to be called with the same arguments. This should be squashed in with the "Merge strategies" patch when committed, I'm just keeping this separate for review clarity.
Created attachment 199390 [details] [review] st-texture-cache: Rearrange code to prevent some work and a memory leak For some reason, the texture cache decides to make a request and then look up an icon in the icon theme. If it's valid, it just returns, fine, but if it doesn't add the icon, it tries to undo the request, leaking an AsyncTextureLoadData that isn't freed in the process.
Created attachment 199391 [details] [review] st-texture-cache: Don't load a fallback pixbuf on the main thread We create lots of random requests when searching, so this might just help performance there.
Review of attachment 199388 [details] [review]: Looks obviously right.
Comment on attachment 199388 [details] [review] st-texture-cache: Use ANSI C-style comments Attachment 199388 [details] pushed as 5d25716 - st-texture-cache: Use ANSI C-style comments OK.
Review of attachment 199387 [details] [review]: It's not clear to me why there were originally two structures anymore, but I don't see anything wrong looking over this. If it works for you then it's probably fine, though I'd be a bit more confident if you'd said you'd done a run through valgrind. Just two minor comments. ::: src/st/st-texture-cache.c @@ +340,3 @@ + + if (data->key) + g_free (data->key); You don't need the if() here - g_free is NULL safe. Also it'd be nice to order the _destroy calls in the same order as they're declared in the structure. @@ +348,2 @@ + if (data->textures) + g_slist_free (data->textures); g_slist_free is also NULL safe.
Created attachment 199458 [details] [review] st-texture-cache: Merge strategies Rather than have five or six structs allocated duplicating data, just keep one and simplify the code considerably. Again, part of my ongoing quest to merge St and Mx. Rearrange the struct and free order to be a little more logical
Colin, can you review the other patches? I'd like to squash them before pushing.
Review of attachment 199389 [details] [review]: Looks fine.
Review of attachment 199390 [details] [review]: ::: src/st/st-texture-cache.c @@ +1082,2 @@ info = gtk_icon_theme_lookup_by_gicon (theme, icon, size, GTK_ICON_LOOKUP_USE_BUILTIN); + if (info == NULL) Missing a g_free (key). Or move the info lookup to before we calculate the key.
Review of attachment 199391 [details] [review]: I think the reason we did this before is because GtkIconTheme isn't threadsafe, and looking it the code it still appears that's true. See gtkicontheme.c:ensure_valid_themes(). So we might be able to move gtk_icon_theme_load_icon() to the worker thread, but not gtk_icon_theme_lookup_by_gicon()
Review of attachment 199458 [details] [review]: If this is just a cleaned up one from before, still sounds fine.
Created attachment 199569 [details] [review] st-texture-cache: Rearrange code to prevent some work and a memory leak For some reason, the texture cache decides to make a request and then look up an icon in the icon theme. If it's valid, it just returns, fine, but if it doesn't add the icon, it tries to undo the request, leaking an AsyncTextureLoadData that isn't freed in the process. Fixed the memory leak by moving the lookup above the key. You know what they say -- the best way to (fast|simple) code is by doing less.
(In reply to comment #19) > Review of attachment 199391 [details] [review]: > > I think the reason we did this before is because GtkIconTheme isn't threadsafe, > and looking it the code it still appears that's true. See > gtkicontheme.c:ensure_valid_themes(). > > So we might be able to move gtk_icon_theme_load_icon() to the worker thread, > but not gtk_icon_theme_lookup_by_gicon() I think fixing GtkIconTheme is the way to go for this one, filed bug #662319 with a patch.
Review of attachment 199569 [details] [review]: Yep
Attachment 199458 [details] pushed as b7bf712 - st-texture-cache: Merge strategies Attachment 199569 [details] pushed as 0dd4584 - st-texture-cache: Rearrange code to prevent some work and a memory leak Squashes "Remove similar duplicate code" into "Merge strategies". Leaving bug open for the last patch, "Don't load a fallback pixbuf on the main thread"
Created attachment 199690 [details] [review] st-texture-cache: Don't load a fallback pixbuf on the main thread OK, apparently GTK+ is just considered not thread-safe, with no attempt to change this. Grab the global GDK lock before doing loading the pixbuf on another thread.
Review of attachment 199690 [details] [review]: No, this isn't right. Using gdk_threads_enter() requires _your entire program_ to follow the rules for GDK threading. See http://developer.gnome.org/gdk/stable/gdk-Threads.html Unless (or even if) we have evidence showing that icon theme loading is a bottleneck, I don't see adapting all of mutter and gnome shell to these rules as a good idea.