[Cogl] [PATCH 4/5] pipeline-cache: Use a special trimmed down pipeline for the key
Robert Bragg
robert at sixbynine.org
Wed Apr 3 17:01:37 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:
> When a pipeline is added to the cache, a normal copy would previously be
> made to use as the key in the hash table. This copy keeps a reference
> to the real pipeline which means all of the resources it contains are
> retained forever, even if they aren't necessary to generate the hash.
>
> This patch changes it to create a trimmed down copy that only has the
> state necessary to generate the hash. A new function called
> _cogl_pipeline_deep_copy is added which makes a new pipeline that is
> directly a child of the root pipeline. It then copies over the
> pertinent state from the original pipeline. The pipeline state is
> copied using the existing _cogl_pipeline_copy_differences function.
> There was no equivalent function for the layer state so I have added
> one.
>
> That way the pipeline key doesn't have the texture data state and it
> doesn't hold a reference to the original pipeline so it should be much
> cheaper to keep around.
> ---
> cogl/cogl-pipeline-hash-table.c | 25 ++++-----
> cogl/cogl-pipeline-layer-private.h | 5 ++
> cogl/cogl-pipeline-layer.c | 103 +++++++++++++++++++++++++++++++++++++
> cogl/cogl-pipeline-private.h | 11 ++++
> cogl/cogl-pipeline.c | 91 ++++++++++++++++++++++++++++++++
> tests/conform/test-conform-main.c | 2 +-
> 6 files changed, 222 insertions(+), 15 deletions(-)
>
> diff --git a/cogl/cogl-pipeline-hash-table.c b/cogl/cogl-pipeline-hash-table.c
> index 8df6371..8921efc 100644
> --- a/cogl/cogl-pipeline-hash-table.c
> +++ b/cogl/cogl-pipeline-hash-table.c
> @@ -130,24 +130,21 @@ _cogl_pipeline_hash_table_get (CoglPipelineHashTable *hash,
> "unusual, so something is probably wrong!\n",
> hash->debug_string);
>
> - /* 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;
> entry->hash_value = dummy_entry.hash_value;
>
> + copy_state = hash->main_state;
> + if (hash->layer_state)
> + copy_state |= COGL_PIPELINE_STATE_LAYERS;
> +
> + /* Create a new pipeline that is a child of the root pipeline
> + * instead of a normal copy so that the template pipeline won't hold
> + * a reference to the original pipeline */
> + entry->pipeline = _cogl_pipeline_deep_copy (key_pipeline,
> + copy_state,
> + hash->layer_state);
> +
> g_hash_table_insert (hash->table, entry, entry);
>
> hash->n_unique_pipelines++;
> diff --git a/cogl/cogl-pipeline-layer-private.h b/cogl/cogl-pipeline-layer-private.h
> index ca69168..7c493da 100644
> --- a/cogl/cogl-pipeline-layer-private.h
> +++ b/cogl/cogl-pipeline-layer-private.h
> @@ -347,6 +347,11 @@ _cogl_pipeline_layer_get_wrap_mode_t (CoglPipelineLayer *layer);
> CoglPipelineWrapMode
> _cogl_pipeline_layer_get_wrap_mode_p (CoglPipelineLayer *layer);
>
> +void
> +_cogl_pipeline_layer_copy_differences (CoglPipelineLayer *dest,
> + CoglPipelineLayer *src,
> + unsigned long differences);
> +
> unsigned long
> _cogl_pipeline_layer_compare_differences (CoglPipelineLayer *layer0,
> CoglPipelineLayer *layer1);
> diff --git a/cogl/cogl-pipeline-layer.c b/cogl/cogl-pipeline-layer.c
> index a3a4545..b08d456 100644
> --- a/cogl/cogl-pipeline-layer.c
> +++ b/cogl/cogl-pipeline-layer.c
> @@ -42,6 +42,8 @@
> #include "cogl-context-private.h"
> #include "cogl-texture-private.h"
>
> +#include <string.h>
> +
> static void
> _cogl_pipeline_layer_free (CoglPipelineLayer *layer);
>
> @@ -146,6 +148,107 @@ _cogl_get_n_args_for_combine_func (CoglPipelineCombineFunc func)
> return 0;
> }
>
> +void
> +_cogl_pipeline_layer_copy_differences (CoglPipelineLayer *dest,
> + CoglPipelineLayer *src,
> + unsigned long differences)
> +{
> + CoglPipelineLayerBigState *big_dest, *big_src;
> +
> + if ((differences & COGL_PIPELINE_LAYER_STATE_NEEDS_BIG_STATE) &&
> + !dest->has_big_state)
> + {
> + dest->big_state = g_slice_new (CoglPipelineLayerBigState);
> + dest->has_big_state = TRUE;
> + }
> +
> + big_dest = dest->big_state;
> + big_src = src->big_state;
> +
> + dest->differences |= differences;
> +
> + while (differences)
> + {
> + int index = _cogl_util_ffs (differences) - 1;
> +
> + differences &= ~(1 << index);
> +
> + /* This convoluted switch statement is just here so that we'll
> + * get a warning if a new state is added without handling it
> + * here */
> + switch (index)
> + {
> + case COGL_PIPELINE_LAYER_STATE_COUNT:
> + case COGL_PIPELINE_LAYER_STATE_UNIT_INDEX:
> + g_warn_if_reached ();
> + break;
> +
> + case COGL_PIPELINE_LAYER_STATE_TEXTURE_TYPE_INDEX:
> + dest->texture_type = src->texture_type;
> + break;
> +
> + case COGL_PIPELINE_LAYER_STATE_TEXTURE_DATA_INDEX:
> + dest->texture = src->texture;
> + if (dest->texture)
> + cogl_object_ref (dest->texture);
> + break;
> +
> + case COGL_PIPELINE_LAYER_STATE_SAMPLER_INDEX:
> + dest->sampler_cache_entry = src->sampler_cache_entry;
> + break;
> +
> + case COGL_PIPELINE_LAYER_STATE_COMBINE_INDEX:
> + {
> + CoglPipelineCombineFunc func;
> + int n_args, i;
> +
> + func = big_src->texture_combine_rgb_func;
> + big_dest->texture_combine_rgb_func = func;
> + n_args = _cogl_get_n_args_for_combine_func (func);
> + for (i = 0; i < n_args; i++)
> + {
> + big_dest->texture_combine_rgb_src[i] =
> + big_src->texture_combine_rgb_src[i];
> + big_dest->texture_combine_rgb_op[i] =
> + big_src->texture_combine_rgb_op[i];
> + }
> +
> + func = big_src->texture_combine_alpha_func;
> + big_dest->texture_combine_alpha_func = func;
> + n_args = _cogl_get_n_args_for_combine_func (func);
> + for (i = 0; i < n_args; i++)
> + {
> + big_dest->texture_combine_alpha_src[i] =
> + big_src->texture_combine_alpha_src[i];
> + big_dest->texture_combine_alpha_op[i] =
> + big_src->texture_combine_alpha_op[i];
> + }
> + }
> + break;
> +
> + case COGL_PIPELINE_LAYER_STATE_COMBINE_CONSTANT_INDEX:
> + memcpy (big_dest->texture_combine_constant,
> + big_src->texture_combine_constant,
> + sizeof (big_dest->texture_combine_constant));
> + break;
> +
> + case COGL_PIPELINE_LAYER_STATE_POINT_SPRITE_COORDS_INDEX:
> + big_dest->point_sprite_coords = big_src->point_sprite_coords;
> + break;
> +
> + case COGL_PIPELINE_LAYER_STATE_VERTEX_SNIPPETS_INDEX:
> + _cogl_pipeline_snippet_list_copy (&big_dest->vertex_snippets,
> + &big_src->vertex_snippets);
> + break;
> +
> + case COGL_PIPELINE_LAYER_STATE_FRAGMENT_SNIPPETS_INDEX:
> + _cogl_pipeline_snippet_list_copy (&big_dest->fragment_snippets,
> + &big_src->fragment_snippets);
> + break;
> + }
> + }
> +}
> +
> static void
> _cogl_pipeline_layer_init_multi_property_sparse_state (
> CoglPipelineLayer *layer,
> diff --git a/cogl/cogl-pipeline-private.h b/cogl/cogl-pipeline-private.h
> index 32f6d51..67fe5e4 100644
> --- a/cogl/cogl-pipeline-private.h
> +++ b/cogl/cogl-pipeline-private.h
> @@ -787,6 +787,17 @@ _cogl_pipeline_hash (CoglPipeline *pipeline,
> unsigned long layer_differences,
> CoglPipelineEvalFlags flags);
>
> +/* Makes a copy of the given pipeline that is a child of the root
> + * pipeline rather than a child of the source pipeline. That way the
> + * new pipeline won't hold a reference to the source pipeline. The
> + * differences specified in @differences and @layer_differences are
> + * copied across and all other state is left with the default
> + * values. */
> +CoglPipeline *
> +_cogl_pipeline_deep_copy (CoglPipeline *pipeline,
> + unsigned long differences,
> + unsigned long layer_differences);
> +
> CoglPipeline *
> _cogl_pipeline_journal_ref (CoglPipeline *pipeline);
>
> diff --git a/cogl/cogl-pipeline.c b/cogl/cogl-pipeline.c
> index 314cf44..c397eb5 100644
> --- a/cogl/cogl-pipeline.c
> +++ b/cogl/cogl-pipeline.c
> @@ -2522,6 +2522,97 @@ _cogl_pipeline_hash (CoglPipeline *pipeline,
>
> typedef struct
> {
> + CoglContext *context;
> + CoglPipeline *src_pipeline;
> + CoglPipeline *dst_pipeline;
> + unsigned int layer_differences;
> +} DeepCopyData;
> +
> +static CoglBool
> +deep_copy_layer_cb (CoglPipelineLayer *src_layer,
> + void *user_data)
> +{
> + DeepCopyData *data = user_data;
> + CoglPipelineLayer *dst_layer;
> + unsigned int differences = data->layer_differences;
> +
> + dst_layer = _cogl_pipeline_get_layer (data->dst_pipeline, src_layer->index);
> +
> + while (src_layer != data->context->default_layer_n &&
> + src_layer != data->context->default_layer_0 &&
> + differences)
> + {
> + unsigned long to_copy = differences & src_layer->differences;
> +
> + if (to_copy)
> + {
> + _cogl_pipeline_layer_copy_differences (dst_layer, src_layer, to_copy);
> + differences ^= to_copy;
> + }
> +
> + src_layer = COGL_PIPELINE_LAYER (COGL_NODE (src_layer)->parent);
> + }
> +
> + return TRUE;
> +}
> +
> +CoglPipeline *
> +_cogl_pipeline_deep_copy (CoglPipeline *pipeline,
> + unsigned long differences,
> + unsigned long layer_differences)
> +{
> + CoglPipeline *new, *authority;
> + CoglBool copy_layer_state;
> +
> + _COGL_GET_CONTEXT (ctx, NULL);
> +
> + if ((differences & COGL_PIPELINE_STATE_LAYERS))
> + {
> + copy_layer_state = TRUE;
> + differences &= ~COGL_PIPELINE_STATE_LAYERS;
> + }
> + else
> + copy_layer_state = FALSE;
> +
> + new = cogl_pipeline_new (ctx);
> +
> + for (authority = pipeline;
> + authority != ctx->default_pipeline && differences;
> + authority = COGL_PIPELINE (COGL_NODE (authority)->parent))
> + {
> + unsigned long to_copy = differences & authority->differences;
> +
> + if (to_copy)
> + {
> + _cogl_pipeline_copy_differences (new, authority, to_copy);
> + differences ^= to_copy;
> + }
> + }
> +
> + if (copy_layer_state)
> + {
> + DeepCopyData data;
> +
> + /* The unit index doesn't need to be copied because it should
> + * end up with the same values anyway because the new pipeline
> + * will have the same indices as the source pipeline */
> + layer_differences &= ~COGL_PIPELINE_LAYER_STATE_UNIT;
> +
> + data.context = ctx;
> + data.src_pipeline = pipeline;
> + data.dst_pipeline = new;
> + data.layer_differences = layer_differences;
> +
> + _cogl_pipeline_foreach_layer_internal (pipeline,
> + deep_copy_layer_cb,
> + &data);
> + }
> +
> + return new;
> +}
> +
> +typedef struct
> +{
> int i;
> CoglPipelineLayer **layers;
> } AddLayersToArrayState;
> diff --git a/tests/conform/test-conform-main.c b/tests/conform/test-conform-main.c
> index 2dd5655..9345065 100644
> --- a/tests/conform/test-conform-main.c
> +++ b/tests/conform/test-conform-main.c
> @@ -122,7 +122,7 @@ main (int argc, char **argv)
>
> ADD_TEST (test_copy_replace_texture, 0, 0);
>
> - ADD_TEST (test_pipeline_cache_unrefs_texture, 0, TEST_KNOWN_FAILURE);
> + ADD_TEST (test_pipeline_cache_unrefs_texture, 0, 0);
>
> UNPORTED_TEST (test_viewport);
>
> --
> 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