GNOME Bugzilla – Bug 736936
Use GResources for theme loading
Last modified: 2014-10-15 01:56:50 UTC
Currently, the Shell theme uses files in $pkgdatadir. Move these to a GResource will make startup and loading faster.
Created attachment 286543 [details] [review] background: use GFiles instead of filenames We want to use GResources for system backgrounds, so move this to a GFile.
Created attachment 286544 [details] [review] texture-cache: remove unused base64 code path This was introduced in b0c6cf3f to support image data for web search providers, which do not exist anymore. Just remove it.
Created attachment 286545 [details] [review] st: always use GFile internally We're moving the theme infrastructure towards GResource, so as a first step move all the loading to use GFiles instead of URIs or paths.
Created attachment 286546 [details] [review] background: port to new GFile MetaBackground API
Created attachment 286547 [details] [review] theme: convert stylesheet loading to GFile In preparation to making it a GResource.
Created attachment 286548 [details] [review] theme: make a GResource Now that we have all the infrastructure ready, port the theme to a GResource.
Review of attachment 286544 [details] [review]: I swore I removed this at some point, or at least tried to. Perhaps this is needed for the message tray? I'm fine with removing it after the freeze and seeing what turns up.
Review of attachment 286546 [details] [review]: I assume you need to file mutter patches for this? ::: js/ui/background.js @@ +128,3 @@ +function _fileEqual0(file1, file2) { + if (!file1 && !file2) + return true; I'd write this as: if (file1 == file2) return true;
Review of attachment 286547 [details] [review]: Technically, you should update tests/testcommon/ui.js, but I'd be surprised if those tests still run anyway. ::: src/st/st-theme.c @@ +103,3 @@ + GFile *file2) +{ + if ((file1 == NULL) && (file2 == NULL)) Again: if (file1 == file2) return TRUE;
Review of attachment 286543 [details] [review]: Oh, sorry, I missed this mutter patch. I didn't realize we were going to move GResources to a system background. I assume it's for noise-texture.png? ::: src/compositor/meta-background-image.c @@ +135,3 @@ + + pixbuf = gdk_pixbuf_new_from_stream (G_INPUT_STREAM (stream), NULL, + &error); Same line, please. ::: src/compositor/meta-background.c @@ +246,3 @@ + GFile *file2) +{ + if ((file1 == NULL) && (file2 == NULL)) Yikes, I keep finding this code all over the place. You know what to do :)
Review of attachment 286545 [details] [review]: ::: src/st/st-texture-cache.c @@ +1181,1 @@ key = g_strconcat (CACHE_PREFIX_URI, uri, NULL); I wonder if we should use g_file_hash instead. ::: src/st/st-theme-node.c @@ +3852,3 @@ + if ((node->background_image != NULL) && + (other->background_image != NULL) && + !g_file_equal (node->background_image, other->background_image)) Not gonna go for another file_equal0? :)
Review of attachment 286548 [details] [review]: Is there any reason not to put it in the same resource as all the others? So we just have a gnome-shell.gresource instead of a -js and a -theme one? Also, I assume you double-checked that the CSS file and assets are actually being loaded from the resource and not the disk? I noticed you didn't actually touch _st_theme_resolve_uri, so you should probably triple-check that one. ::: js/ui/extensionSystem.js @@ +119,3 @@ if (stylesheetFile.query_exists(null)) { let theme = St.ThemeContext.get_for_stage(global.stage).get_theme(); + theme.load_stylesheet(stylesheetFile); I feel like this should be in an earlier patch...
Created attachment 286552 [details] [review] background: use GFiles instead of filenames We want to use GResources for system backgrounds, so move this to a GFile.
Created attachment 286553 [details] [review] st: always use GFile internally We're moving the theme infrastructure towards GResource, so as a first step move all the loading to use GFiles instead of URIs or paths.
Created attachment 286554 [details] [review] background: port to new GFile MetaBackground API
Created attachment 286555 [details] [review] theme: convert stylesheet loading to GFile In preparation to making it a GResource.
Created attachment 286556 [details] [review] theme: make a GResource Now that we have all the infrastructure ready, port the theme to a GResource.
Created attachment 286559 [details] [review] st: always use GFile internally We're moving the theme infrastructure towards GResource, so as a first step move all the loading to use GFiles instead of URIs or paths.
Created attachment 286561 [details] [review] theme: convert stylesheet loading to GFile In preparation to making it a GResource.
Thanks for the reviews Jasper. I think I addressed all of your points, except for a couple: - I don't believe the data: stuff is used by the message tray. At least if it does, it's totally non-obvious... should be fine to remove for now. - the background stuff was moved to GFile exactly for noise-texture.png - I did not make these a single GResource because it would be hard to build. If you look at how we make the JS GResource, some files are actually generated at build time, and then not included in the dist, which would be harder to do across two different directories. If you strongly feel for this, I can work on it, otherwise I think it could be punted to a future improvement - new patchset also fixes tests
Review of attachment 286552 [details] [review]: OK.
Review of attachment 286561 [details] [review]: Looks good.
Review of attachment 286554 [details] [review]: OK.
Review of attachment 286556 [details] [review]: Yep.
Created attachment 286652 [details] [review] st: always use GFile internally -- Minor change to the test
Review of attachment 286652 [details] [review]: Oh, yep, nice catch.
(In reply to comment #0) > Currently, the Shell theme uses files in $pkgdatadir. Move these to a GResource > will make startup and loading faster. Curious - do you have any data for this?
Our computer uses a spinning disk, and moving JS files to a resource cut a good 3-4 seconds off of startup time. We're hoping the same will happen with themes and theme assets.
(In reply to comment #29) > Our computer uses a spinning disk, and moving JS files to a resource cut a good > 3-4 seconds off of startup time. We're hoping the same will happen with themes > and theme assets. Shell currrently has 133 JS files - assuming pessimistically 1 seek per file, 15ms each seek, that would be 2.0sec. It's pretty grim if things are twice as bad as my "pessimistic" estimates. This would imply that you have some serious fragmentation or layout issues. Since you aren't going to get away from having to read a lot of files on startup it's certainly something you mght want to a investigate at some point - look at a directory of files from a single package - where are the blocks for those files physically on disk? (ostree is possibly problematical here since it will replace subsets of files in a directory and not touch others ... even on a SSD there seem to be considerable gradual-slowdown effects showing up on perf.gnome.org. Though actual users won't be doing thousands of ostree pulls.) This isn't to say that even if you gain just a few fractions of a second from this (36 files in the GNOME upstream theme) it's a bad idea - just that 3-4 sec number makes me go "hmmmm".
Comment on attachment 286552 [details] [review] background: use GFiles instead of filenames Attachment 286552 [details] pushed as a37f632 - background: use GFiles instead of filenames
Attachment 286544 [details] pushed as 2dc41c9 - texture-cache: remove unused base64 code path Attachment 286554 [details] pushed as 38add2e - background: port to new GFile MetaBackground API Attachment 286556 [details] pushed as 49c4ba5 - theme: make a GResource Attachment 286561 [details] pushed as 642bf2b - theme: convert stylesheet loading to GFile Attachment 286652 [details] pushed as 328bb1c - st: always use GFile internally