[Cogl] [PATCH 3/5] pipeline-hash-table: Store the hash value in the entry

[Robert Bragg]

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:
> Calculating the hash value for a pipeline can be a bit expensive.
> Previously when adding a new pipeline to the hash table we would end
> up calculating the hash value once when checking whether the pipeline
> is already in the hash table and then again when adding the pipeline
> to the hash table. Ideally GHashTable would provide some API to add an
> entry with a precalculated hash value to avoid the recalculation, but
> seeing as it doesn't do that we can force it to avoid recalculating by
> storing the hash value as part of the struct we are using for the key.
> That way the hash func passed to GHashTable can simply return the
> precalculated value and we can calculate the hash outside of the
> GHashTable calls.
> ---
>  cogl/cogl-pipeline-hash-table.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/cogl/cogl-pipeline-hash-table.c b/cogl/cogl-pipeline-hash-table.c
> index d3769f3..8df6371 100644
> --- a/cogl/cogl-pipeline-hash-table.c
> +++ b/cogl/cogl-pipeline-hash-table.c
> @@ -38,6 +38,12 @@ typedef struct
>    /* The template pipeline */
>    CoglPipeline *pipeline;
>
> +  /* Calculating the hash is a little bit expensive for pipelines so
> +   * we don't want to do it repeatedly for entries that are already in
> +   * the hash table. Instead we cache the value here and calculate it
> +   * outside of the GHashTable. */
> +  unsigned int hash_value;
> +
>    /* 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
> @@ -59,12 +65,8 @@ 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);
> +  return entry->hash_value;
>  }
>
>  static CoglBool
> @@ -114,7 +116,10 @@ _cogl_pipeline_hash_table_get (CoglPipelineHashTable *hash,
>
>    dummy_entry.pipeline = key_pipeline;
>    dummy_entry.hash = hash;
> -
> +  dummy_entry.hash_value = _cogl_pipeline_hash (key_pipeline,
> +                                                hash->main_state,
> +                                                hash->layer_state,
> +                                                0);
>    entry = g_hash_table_lookup (hash->table, &dummy_entry);
>
>    if (entry)
> @@ -125,13 +130,6 @@ _cogl_pipeline_hash_table_get (CoglPipelineHashTable *hash,
>                 "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.
> @@ -148,6 +146,7 @@ _cogl_pipeline_hash_table_get (CoglPipelineHashTable *hash,
>    entry = g_slice_new (CoglPipelineHashTableEntry);
>    entry->pipeline = cogl_pipeline_copy (key_pipeline);
>    entry->hash = hash;
> +  entry->hash_value = dummy_entry.hash_value;
>
>    g_hash_table_insert (hash->table, entry, entry);
>
> --
> 1.7.11.3.g3c3efa5
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl