pipeline-cache: Use a shared hash table wrapper

The pipeline cache contains three separate hash tables, one for the
state affecting the vertex shaders, one for the fragment shaders and
one for the resulting combined program. Previously these hash tables
had a fair bit of duplicated code to calculate the hashes, check for
equality and copy the pipeline when it is added. This patch moves the
common bits of code to a new type called CoglPipelineHashTable which
just wraps a GHashTable with a given set of state flags to use for
hashing and checking for equality.

Reviewed-by: Robert Bragg <robert@linux.intel.com>

(cherry picked from commit 402796430c839038339e531363b8c2463f9b2a9e)

Conflicts:
	cogl/Makefile.am

cogl/Makefile.am

@@ -349,6 +349,8 @@ cogl_sources_c = \
 	$(srcdir)/cogl-pipeline-snippet.c		\
 	$(srcdir)/cogl-pipeline-cache.h			\
 	$(srcdir)/cogl-pipeline-cache.c			\
+	$(srcdir)/cogl-pipeline-hash-table.h		\
+	$(srcdir)/cogl-pipeline-hash-table.c		\
 	$(srcdir)/cogl-material-compat.c		\
 	$(srcdir)/cogl-program.c			\
 	$(srcdir)/cogl-program-private.h		\

cogl/cogl-pipeline-cache.c

@@ -3,7 +3,7 @@
  *
  * An object oriented GL/GLES Abstraction/Utility Layer
  *
- * Copyright (C) 2011 Intel Corporation.
+ * Copyright (C) 2011, 2013 Intel Corporation.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -32,133 +32,47 @@
 #include "cogl-context-private.h"
 #include "cogl-pipeline-private.h"
 #include "cogl-pipeline-cache.h"
+#include "cogl-pipeline-hash-table.h"
 
 struct _CoglPipelineCache
 {
-  GHashTable *fragment_hash;
+  CoglPipelineHashTable fragment_hash;
-  GHashTable *vertex_hash;
+  CoglPipelineHashTable vertex_hash;
-  GHashTable *combined_hash;
+  CoglPipelineHashTable combined_hash;
 };
 
-static unsigned int
-pipeline_fragment_hash (const void *data)
-{
-  unsigned int fragment_state;
-  unsigned int layer_fragment_state;
-
-  _COGL_GET_CONTEXT (ctx, 0);
-
-  fragment_state =
-    _cogl_pipeline_get_state_for_fragment_codegen (ctx);
-  layer_fragment_state =
-    _cogl_pipeline_get_layer_state_for_fragment_codegen (ctx);
-
-  return _cogl_pipeline_hash ((CoglPipeline *)data,
-                              fragment_state, layer_fragment_state,
-                              0);
-}
-
-static CoglBool
-pipeline_fragment_equal (const void *a, const void *b)
-{
-  unsigned int fragment_state;
-  unsigned int layer_fragment_state;
-
-  _COGL_GET_CONTEXT (ctx, 0);
-
-  fragment_state =
-    _cogl_pipeline_get_state_for_fragment_codegen (ctx);
-  layer_fragment_state =
-    _cogl_pipeline_get_layer_state_for_fragment_codegen (ctx);
-
-  return _cogl_pipeline_equal ((CoglPipeline *)a, (CoglPipeline *)b,
-                               fragment_state, layer_fragment_state,
-                               0);
-}
-
-static unsigned int
-pipeline_vertex_hash (const void *data)
-{
-  unsigned long vertex_state =
-    COGL_PIPELINE_STATE_AFFECTS_VERTEX_CODEGEN;
-  unsigned long layer_vertex_state =
-    COGL_PIPELINE_LAYER_STATE_AFFECTS_VERTEX_CODEGEN;
-
-  return _cogl_pipeline_hash ((CoglPipeline *)data,
-                              vertex_state, layer_vertex_state,
-                              0);
-}
-
-static CoglBool
-pipeline_vertex_equal (const void *a, const void *b)
-{
-  unsigned long vertex_state =
-    COGL_PIPELINE_STATE_AFFECTS_VERTEX_CODEGEN;
-  unsigned long layer_vertex_state =
-    COGL_PIPELINE_LAYER_STATE_AFFECTS_VERTEX_CODEGEN;
-
-  return _cogl_pipeline_equal ((CoglPipeline *)a, (CoglPipeline *)b,
-                               vertex_state, layer_vertex_state,
-                               0);
-}
-
-static unsigned int
-pipeline_combined_hash (const void *data)
-{
-  unsigned int combined_state;
-  unsigned int layer_combined_state;
-
-  _COGL_GET_CONTEXT (ctx, 0);
-
-  combined_state =
-    _cogl_pipeline_get_state_for_fragment_codegen (ctx) |
-    COGL_PIPELINE_STATE_AFFECTS_VERTEX_CODEGEN;
-  layer_combined_state =
-    _cogl_pipeline_get_layer_state_for_fragment_codegen (ctx) |
-    COGL_PIPELINE_LAYER_STATE_AFFECTS_VERTEX_CODEGEN;
-
-  return _cogl_pipeline_hash ((CoglPipeline *)data,
-                              combined_state, layer_combined_state,
-                              0);
-}
-
-static CoglBool
-pipeline_combined_equal (const void *a, const void *b)
-{
-  unsigned int combined_state;
-  unsigned int layer_combined_state;
-
-  _COGL_GET_CONTEXT (ctx, 0);
-
-  combined_state =
-    _cogl_pipeline_get_state_for_fragment_codegen (ctx) |
-    COGL_PIPELINE_STATE_AFFECTS_VERTEX_CODEGEN;
-  layer_combined_state =
-    _cogl_pipeline_get_layer_state_for_fragment_codegen (ctx) |
-    COGL_PIPELINE_LAYER_STATE_AFFECTS_VERTEX_CODEGEN;
-
-  return _cogl_pipeline_equal ((CoglPipeline *)a, (CoglPipeline *)b,
-                               combined_state, layer_combined_state,
-                               0);
-}
-
 CoglPipelineCache *
 _cogl_pipeline_cache_new (void)
 {
   CoglPipelineCache *cache = g_new (CoglPipelineCache, 1);
+  unsigned long vertex_state;
+  unsigned long layer_vertex_state;
+  unsigned int fragment_state;
+  unsigned int layer_fragment_state;
 
-  cache->fragment_hash = g_hash_table_new_full (pipeline_fragment_hash,
+  _COGL_GET_CONTEXT (ctx, 0);
-                                                pipeline_fragment_equal,
+
-                                                cogl_object_unref,
+  vertex_state =
-                                                cogl_object_unref);
+    COGL_PIPELINE_STATE_AFFECTS_VERTEX_CODEGEN;
-  cache->vertex_hash = g_hash_table_new_full (pipeline_vertex_hash,
+  layer_vertex_state =
-                                              pipeline_vertex_equal,
+    COGL_PIPELINE_LAYER_STATE_AFFECTS_VERTEX_CODEGEN;
-                                              cogl_object_unref,
+  fragment_state =
-                                              cogl_object_unref);
+    _cogl_pipeline_get_state_for_fragment_codegen (ctx);
-  cache->combined_hash = g_hash_table_new_full (pipeline_combined_hash,
+  layer_fragment_state =
-                                                pipeline_combined_equal,
+    _cogl_pipeline_get_layer_state_for_fragment_codegen (ctx);
-                                                cogl_object_unref,
+
-                                                cogl_object_unref);
+  _cogl_pipeline_hash_table_init (&cache->vertex_hash,
+                                  vertex_state,
+                                  layer_vertex_state,
+                                  "vertex shaders");
+  _cogl_pipeline_hash_table_init (&cache->fragment_hash,
+                                  fragment_state,
+                                  layer_fragment_state,
+                                  "fragment shaders");
+  _cogl_pipeline_hash_table_init (&cache->combined_hash,
+                                  vertex_state | fragment_state,
+                                  layer_vertex_state | layer_fragment_state,
+                                  "programs");
 
   return cache;
 }
@@ -166,9 +80,9 @@ _cogl_pipeline_cache_new (void)
 void
 _cogl_pipeline_cache_free (CoglPipelineCache *cache)
 {
-  g_hash_table_destroy (cache->fragment_hash);
+  _cogl_pipeline_hash_table_destroy (&cache->fragment_hash);
-  g_hash_table_destroy (cache->vertex_hash);
+  _cogl_pipeline_hash_table_destroy (&cache->vertex_hash);
-  g_hash_table_destroy (cache->combined_hash);
+  _cogl_pipeline_hash_table_destroy (&cache->combined_hash);
   g_free (cache);
 }
 
@@ -176,107 +90,22 @@ CoglPipeline *
 _cogl_pipeline_cache_get_fragment_template (CoglPipelineCache *cache,
                                             CoglPipeline *key_pipeline)
 {
-  CoglPipeline *template =
+  return _cogl_pipeline_hash_table_get (&cache->fragment_hash,
-    g_hash_table_lookup (cache->fragment_hash, key_pipeline);
+                                        key_pipeline);
-
-  if (template == NULL)
-    {
-      /* XXX: I wish there was a way to insert into a GHashTable with
-       * a pre-calculated hash value since there is a cost to
-       * calculating the hash of a CoglPipeline and in this case we
-       * know we have already called _cogl_pipeline_hash during the
-       * lookup so we could pass the value through to here to avoid
-       * hashing it again.
-       */
-
-      /* XXX: Any keys referenced by the hash table need to remain
-       * valid all the while that there are corresponding values,
-       * so for now we simply make a copy of the current authority
-       * pipeline.
-       *
-       * FIXME: A problem with this is that our key into the cache may
-       * hold references to some arbitrary user textures which will
-       * now be kept alive indefinitly which is a shame. A better
-       * solution will be to derive a special "key pipeline" from the
-       * authority which derives from the base Cogl pipeline (to avoid
-       * affecting the lifetime of any other pipelines) and only takes
-       * a copy of the state that relates to the fragment shader and
-       * references small dummy textures instead of potentially large
-       * user textures. */
-      template = cogl_pipeline_copy (key_pipeline);
-
-      g_hash_table_insert (cache->fragment_hash,
-                           template,
-                           cogl_object_ref (template));
-
-      if (G_UNLIKELY (g_hash_table_size (cache->fragment_hash) > 50))
-        {
-          static CoglBool seen = FALSE;
-          if (!seen)
-            g_warning ("Over 50 separate fragment shaders have been "
-                       "generated which is very unusual, so something "
-                       "is probably wrong!\n");
-          seen = TRUE;
-        }
-    }
-
-  return template;
 }
 
 CoglPipeline *
 _cogl_pipeline_cache_get_vertex_template (CoglPipelineCache *cache,
                                           CoglPipeline *key_pipeline)
 {
-  CoglPipeline *template =
+  return _cogl_pipeline_hash_table_get (&cache->vertex_hash,
-    g_hash_table_lookup (cache->vertex_hash, key_pipeline);
+                                        key_pipeline);
-
-  if (template == NULL)
-    {
-      template = cogl_pipeline_copy (key_pipeline);
-
-      g_hash_table_insert (cache->vertex_hash,
-                           template,
-                           cogl_object_ref (template));
-
-      if (G_UNLIKELY (g_hash_table_size (cache->vertex_hash) > 50))
-        {
-          static CoglBool seen = FALSE;
-          if (!seen)
-            g_warning ("Over 50 separate vertex shaders have been "
-                       "generated which is very unusual, so something "
-                       "is probably wrong!\n");
-          seen = TRUE;
-        }
-    }
-
-  return template;
 }
 
 CoglPipeline *
 _cogl_pipeline_cache_get_combined_template (CoglPipelineCache *cache,
                                             CoglPipeline *key_pipeline)
 {
-  CoglPipeline *template =
+  return _cogl_pipeline_hash_table_get (&cache->combined_hash,
-    g_hash_table_lookup (cache->combined_hash, key_pipeline);
+                                        key_pipeline);
-
-  if (template == NULL)
-    {
-      template = cogl_pipeline_copy (key_pipeline);
-
-      g_hash_table_insert (cache->combined_hash,
-                           template,
-                           cogl_object_ref (template));
-
-      if (G_UNLIKELY (g_hash_table_size (cache->combined_hash) > 50))
-        {
-          static CoglBool seen = FALSE;
-          if (!seen)
-            g_warning ("Over 50 separate programs have been "
-                       "generated which is very unusual, so something "
-                       "is probably wrong!\n");
-          seen = TRUE;
-        }
-    }
-
-  return template;
 }

cogl/cogl-pipeline-hash-table.c

@@ -0,0 +1,157 @@
+/*
+ * Cogl
+ *
+ * An object oriented GL/GLES Abstraction/Utility Layer
+ *
+ * Copyright (C) 2013 Intel Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ *
+ * Authors:
+ *   Neil Roberts <neil@linux.intel.com>
+ *   Robert Bragg <robert@linux.intel.com>
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "cogl-context-private.h"
+#include "cogl-pipeline-private.h"
+#include "cogl-pipeline-hash-table.h"
+
+typedef struct
+{
+  /* The template pipeline */
+  CoglPipeline *pipeline;
+
+  /* GHashTable annoyingly doesn't let us pass a user data pointer to
+   * the hash and equal functions so to work around it we have to
+   * store the pointer in every hash table entry. We will use this
+   * entry as both the key and the value */
+  CoglPipelineHashTable *hash;
+} CoglPipelineHashTableEntry;
+
+static void
+value_destroy_cb (void *value)
+{
+  CoglPipelineHashTableEntry *entry = value;
+
+  cogl_object_unref (entry->pipeline);
+
+  g_slice_free (CoglPipelineHashTableEntry, entry);
+}
+
+static unsigned int
+entry_hash (const void *data)
+{
+  const CoglPipelineHashTableEntry *entry = data;
+  const CoglPipelineHashTable *hash = entry->hash;
+
+  return _cogl_pipeline_hash (entry->pipeline,
+                              hash->main_state,
+                              hash->layer_state,
+                              0);
+}
+
+static CoglBool
+entry_equal (const void *a,
+             const void *b)
+{
+  const CoglPipelineHashTableEntry *entry_a = a;
+  const CoglPipelineHashTableEntry *entry_b = b;
+  const CoglPipelineHashTable *hash = entry_a->hash;
+
+  return _cogl_pipeline_equal (entry_a->pipeline,
+                               entry_b->pipeline,
+                               hash->main_state,
+                               hash->layer_state,
+                               0);
+}
+
+void
+_cogl_pipeline_hash_table_init (CoglPipelineHashTable *hash,
+                                unsigned int main_state,
+                                unsigned int layer_state,
+                                const char *debug_string)
+{
+  hash->n_unique_pipelines = 0;
+  hash->debug_string = debug_string;
+  hash->main_state = main_state;
+  hash->layer_state = layer_state;
+  hash->table = g_hash_table_new_full (entry_hash,
+                                       entry_equal,
+                                       NULL, /* key destroy */
+                                       value_destroy_cb);
+}
+
+void
+_cogl_pipeline_hash_table_destroy (CoglPipelineHashTable *hash)
+{
+  g_hash_table_destroy (hash->table);
+}
+
+CoglPipeline *
+_cogl_pipeline_hash_table_get (CoglPipelineHashTable *hash,
+                               CoglPipeline *key_pipeline)
+{
+  CoglPipelineHashTableEntry dummy_entry;
+  CoglPipelineHashTableEntry *entry;
+  unsigned int copy_state;
+
+  dummy_entry.pipeline = key_pipeline;
+  dummy_entry.hash = hash;
+
+  entry = g_hash_table_lookup (hash->table, &dummy_entry);
+
+  if (entry)
+    return entry->pipeline;
+
+  if (hash->n_unique_pipelines == 50)
+    g_warning ("Over 50 separate %s have been generated which is very "
+               "unusual, so something is probably wrong!\n",
+               hash->debug_string);
+
+  /* XXX: I wish there was a way to insert into a GHashTable with a
+   * pre-calculated hash value since there is a cost to calculating
+   * the hash of a CoglPipeline and in this case we know we have
+   * already called _cogl_pipeline_hash during the lookup so we could
+   * pass the value through to here to avoid hashing it again.
+   */
+
+  /* XXX: Any keys referenced by the hash table need to remain valid
+   * all the while that there are corresponding values, so for now we
+   * simply make a copy of the current authority pipeline.
+   *
+   * FIXME: A problem with this is that our key into the cache may
+   * hold references to some arbitrary user textures which will now be
+   * kept alive indefinitly which is a shame. A better solution will
+   * be to derive a special "key pipeline" from the authority which
+   * derives from the base Cogl pipeline (to avoid affecting the
+   * lifetime of any other pipelines) and only takes a copy of the
+   * state that relates to the fragment shader and references small
+   * dummy textures instead of potentially large user textures.
+   */
+  entry = g_slice_new (CoglPipelineHashTableEntry);
+  entry->pipeline = cogl_pipeline_copy (key_pipeline);
+  entry->hash = hash;
+
+  g_hash_table_insert (hash->table, entry, entry);
+
+  hash->n_unique_pipelines++;
+
+  return entry->pipeline;
+}

cogl/cogl-pipeline-hash-table.h

@@ -0,0 +1,69 @@
+/*
+ * Cogl
+ *
+ * An object oriented GL/GLES Abstraction/Utility Layer
+ *
+ * Copyright (C) 2013 Intel Corporation.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see <http://www.gnu.org/licenses/>.
+ *
+ *
+ */
+
+#ifndef __COGL_PIPELINE_HASH_H__
+#define __COGL_PIPELINE_HASH_H__
+
+#include "cogl-pipeline.h"
+
+typedef struct
+{
+  /* Total number of pipelines that were ever added to the hash. This
+   * is not decremented when a pipeline is removed. It is only used to
+   * generate a warning if an unusually high number of pipelines are
+   * generated */
+  int n_unique_pipelines;
+
+  /* String that will be used to describe the usage of this hash table
+   * in the debug warning when too many pipelines are generated. This
+   * must be a static string because it won't be copied or freed */
+  const char *debug_string;
+
+  unsigned int main_state;
+  unsigned int layer_state;
+
+  GHashTable *table;
+} CoglPipelineHashTable;
+
+void
+_cogl_pipeline_hash_table_init (CoglPipelineHashTable *hash,
+                                unsigned int main_state,
+                                unsigned int layer_state,
+                                const char *debug_string);
+
+void
+_cogl_pipeline_hash_table_destroy (CoglPipelineHashTable *hash);
+
+/*
+ * Gets a pipeline from the hash that has the same state as
+ * @key_pipeline according to the limited state bits passed to
+ * _cogl_pipeline_hash_table_init(). If there is no matching pipelines
+ * already then a copy of key_pipeline is stored in the hash so that
+ * it will be used next time the function is called with a similar
+ * pipeline. In that case the copy itself will be returned
+ */
+CoglPipeline *
+_cogl_pipeline_hash_table_get (CoglPipelineHashTable *hash,
+                               CoglPipeline *key_pipeline);
+
+#endif /* __COGL_PIPELINE_HASH_H__ */