Created attachment 143851 [details] [review]
[ShellTextureCache] Compress multiple load requests

Before, if the texture cache received a request to load say
the themed icon for an application multiple times (as could happen
since we have multiple application displays), it would often create
a thread for each one and in fact, load the pixbuf multiple times.

Avoid this by keeping track of outstanding requests.

Review of attachment 143851 [details] [review]:

General idea looks good, code structure is a bit confusing to me and I think there are some problems in the failure-cleanup code path.

::: src/shell-texture-cache.c
@@ +22,3 @@
 {
   GHashTable *keyed_cache; /* CacheKey -> CoglTexture* */
+  GHashTable *outstanding_requests; /* CacheKey -> AsyncTextureLoadData * */

You need a comment here that this is *only* the outstanding requests for the icon path and that we aren't currently deduplicating requests for uri/thumbnail-uri. (As I understand the code)

@@ +724,3 @@
+  key->policy = data->policy;
+  if (data->icon)
+    key->icon = g_object_ref (data->icon);

It seems to me if you are just assigning all the fields yourself - if there's no abstraction of creation or of the fields - and you are going to dup it anyways, then you might as well just have 'key' be local. (Can initialize with { 0, } to match the new0. Not really as an efficiency thing but so you can just write:

 key.icon = data->icon
 [...]

without all the strdups, refs, and ifs.

@@ +896,3 @@
 
+static gboolean
+create_texture_lookup_status (ShellTextureCache     *cache,

This name is very confusing to me. It's creating a "lookup status?". Is it simply create_texture_and_lookup_status()? (create_texture_and_ensure_request() is what I think I'd call it reading through it.) A comment would help too.

@@ +905,3 @@
+  gboolean had_pending;
+
+  *texture = (ClutterActor*)create_default_texture (cache);

Missing space in 'ClutterActor*'

@@ +924,3 @@
+      /* Not cached and no pending request, create it */
+      pending = g_new0 (AsyncTextureLoadData, 1);
+      g_hash_table_insert (cache->priv->outstanding_requests, cache_key_dup (key), pending);

While I guess the new request is 'pending' too as soon as we insert it, reusing the pending variable here reduces the readability of the code. I think it would be more readable if you just used *request everywhere without the local variable.

@@ +929,3 @@
+  *request = pending;
+  /* Regardless of whether there was a pending request, prepend our texture here. */
+  pending->textures = g_slist_prepend (pending->textures, g_object_ref (*texture));

This is what I mean by readability of the pending variable reuse - if there wasn't a pending request, why are we assigning to pending->textures?

@@ +978,3 @@
     {
+      /* Blah; we failed to find the icon, but we've added our texture to the outstanding
+       * requests.  In that case, just undo what create_texture_lookup_status did.

Strikes me as poor code structure. You are only calling create_texture_lookup_status() from one place, so why not structure things right rather that working around.

I'm not sure that what create_texture_lookup_status() does really makes sense as a single convenience function - in the case where a) the texdata already existed in the cache b) a request already existed, then you want to bail early from *this* function. In the other case, you wnat to continue on *in this function* and do the icon lookupk, and only that succeeds add the request into the list of outstanding requests.

@@ +982,3 @@
+       g_slist_foreach (request->textures, (GFunc) g_object_unref, NULL);
+       g_slist_free (request->textures);
+       g_free (request);

Don't you need to remove the request from ->outstanding_requests too? And probably not free it here since it's owned by outstanding requests?

(In reply to comment #2)
>
> You need a comment here that this is *only* the outstanding requests for the
> icon path and that we aren't currently deduplicating requests for
> uri/thumbnail-uri. (As I understand the code)

Right; ideally we do it for all item types but it's nontrivial, and that's not our issue right now.
 
> I'm not sure that what create_texture_lookup_status() does really makes sense
> as a single convenience function 

Basically we're avoiding deep nested conditionals this way, and gives a starting point for doing this for other items.

> in the case where a) the texdata already
> existed in the cache b) a request already existed, then you want to bail early
> from *this* function. In the other case, you wnat to continue on *in this
> function* and do the icon lookupk, and only that succeeds add the request into
> the list of outstanding requests.

It's hard to avoid duplicate code one way or the other.

Your basic code structure something like:

====
texture = lookup_existing_texture(key); /* A */
if (texture)
   return texture;

texture = create_new_texture(); /* B */
request = lookup_existing_request(key); /* C */
if (request) {
   success = TRUE;
} else {
   request = create_new_request(); /* D */
   
   /* code to setup request to be processed */ /* E */
  
   if (success)
       add_requests_to_outstanding(key, request); /* F */
   else
       free_request();
}

if (success)
    add_texture_to_request(); /* G */
else
    free_texture();

reutrn texture;
====

There's no way you can put A,B,C and F into a utility function and not E and G and have the code be coherent. You could put A,B,C into a utility function but it's going to be a do_a_b_and_c(). The two ways that make sense are:

 A) Pass the code for 'E' into an overallfunction as a callback. This would be a lot easier and more convenient in JS (or C with blocks) than C, but it might work out well if everything that code needs is in the key.

 B) Just write it like above with a number of separate utility functions that have standalone meanings.

Note an update to this patch was committed as ec92bfba14, however it doesn't fully address the comments here, so retitling.

Comment on attachment 143851 [details] [review]
[ShellTextureCache] Compress multiple load requests

updated version of this aptch was committed

We did such compression of icon loads a looong time ago.