[Cogl] [PATCH 2/5] pipeline-cache: Use a shared hash table wrapper
Robert Bragg
robert at sixbynine.org
Wed Apr 3 16:47:08 PDT 2013
This looks good to land to me:
Reviewed-by: Robert Bragg <robert at linux.intel.com>
thanks,
- Robert
On Tue, Mar 26, 2013 at 6:36 PM, Neil Roberts <neil at linux.intel.com> wrote:
> 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.
> ---
> cogl/Makefile.am | 2 +
> cogl/cogl-pipeline-cache.c | 241 ++++++----------------------------------
> cogl/cogl-pipeline-hash-table.c | 157 ++++++++++++++++++++++++++
> cogl/cogl-pipeline-hash-table.h | 69 ++++++++++++
> 4 files changed, 263 insertions(+), 206 deletions(-)
> create mode 100644 cogl/cogl-pipeline-hash-table.c
> create mode 100644 cogl/cogl-pipeline-hash-table.h
>
> diff --git a/cogl/Makefile.am b/cogl/Makefile.am
> index de5f4cc..4e3a268 100644
> --- a/cogl/Makefile.am
> +++ b/cogl/Makefile.am
> @@ -321,6 +321,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-sampler-cache.c \
> $(srcdir)/cogl-sampler-cache-private.h \
> $(srcdir)/cogl-blend-string.c \
> diff --git a/cogl/cogl-pipeline-cache.c b/cogl/cogl-pipeline-cache.c
> index fab3614..df4c433 100644
> --- a/cogl/cogl-pipeline-cache.c
> +++ b/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;
> - GHashTable *vertex_hash;
> - GHashTable *combined_hash;
> + CoglPipelineHashTable fragment_hash;
> + CoglPipelineHashTable vertex_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)
> +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;
>
> _COGL_GET_CONTEXT (ctx, 0);
>
> + vertex_state =
> + COGL_PIPELINE_STATE_AFFECTS_VERTEX_CODEGEN;
> + layer_vertex_state =
> + COGL_PIPELINE_LAYER_STATE_AFFECTS_VERTEX_CODEGEN;
> 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);
> -
> - cache->fragment_hash = g_hash_table_new_full (pipeline_fragment_hash,
> - pipeline_fragment_equal,
> - cogl_object_unref,
> - cogl_object_unref);
> - cache->vertex_hash = g_hash_table_new_full (pipeline_vertex_hash,
> - pipeline_vertex_equal,
> - cogl_object_unref,
> - cogl_object_unref);
> - cache->combined_hash = g_hash_table_new_full (pipeline_combined_hash,
> - pipeline_combined_equal,
> - 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);
> - g_hash_table_destroy (cache->vertex_hash);
> - g_hash_table_destroy (cache->combined_hash);
> + _cogl_pipeline_hash_table_destroy (&cache->fragment_hash);
> + _cogl_pipeline_hash_table_destroy (&cache->vertex_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 =
> - g_hash_table_lookup (cache->fragment_hash, 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;
> + return _cogl_pipeline_hash_table_get (&cache->fragment_hash,
> + key_pipeline);
> }
>
> CoglPipeline *
> _cogl_pipeline_cache_get_vertex_template (CoglPipelineCache *cache,
> CoglPipeline *key_pipeline)
> {
> - CoglPipeline *template =
> - g_hash_table_lookup (cache->vertex_hash, 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;
> + return _cogl_pipeline_hash_table_get (&cache->vertex_hash,
> + key_pipeline);
> }
>
> CoglPipeline *
> _cogl_pipeline_cache_get_combined_template (CoglPipelineCache *cache,
> CoglPipeline *key_pipeline)
> {
> - CoglPipeline *template =
> - g_hash_table_lookup (cache->combined_hash, 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;
> + return _cogl_pipeline_hash_table_get (&cache->combined_hash,
> + key_pipeline);
> }
> diff --git a/cogl/cogl-pipeline-hash-table.c b/cogl/cogl-pipeline-hash-table.c
> new file mode 100644
> index 0000000..d3769f3
> --- /dev/null
> +++ b/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 at linux.intel.com>
> + * Robert Bragg <robert at 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;
> +}
> diff --git a/cogl/cogl-pipeline-hash-table.h b/cogl/cogl-pipeline-hash-table.h
> new file mode 100644
> index 0000000..1b0a0d9
> --- /dev/null
> +++ b/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__ */
> --
> 1.7.11.3.g3c3efa5
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl
More information about the Cogl
mailing list