Re: [PATCH 7/8] d3dx9: Support parameters sharing in effect.

[Matteo Bruni]

2017-04-20 13:26 GMT+02:00 Paul Gofman :
> Signed-off-by: Paul Gofman
>
> dlls/d3dx9_36/d3dx9_private.h |  11 ++-
>  dlls/d3dx9_36/effect.c        | 197 +++++++++++++++++++++++++++++++++++++++++-
>  dlls/d3dx9_36/tests/effect.c  |  17 ++--
>  3 files changed, 211 insertions(+), 14 deletions(-)
>
> diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h
> index da4564d..d6d4280 100644
> --- a/dlls/d3dx9_36/d3dx9_private.h
> +++ b/dlls/d3dx9_36/d3dx9_private.h
> @@ -215,15 +215,22 @@ struct d3dx_parameter
>      struct d3dx_parameter *annotations;
>      struct d3dx_parameter *members;
> -
>
> struct d3dx_parameter *referenced_param;
> struct d3dx_param_eval *param_eval;
>
> struct d3dx_parameter *top_level_param;
> - union
> - {
> -
>    struct d3dx_parameter *referenced_param;
> -
>    struct d3dx_parameter *shared_param;
> - };
> - LONG shared_refcount;
>
> };

I think you mentioned this before but it shouldn't be necessary to
store the whole parameter structure in the pool.
I guess it could be improved after the fact but it shouldn't be too
complicated to do it right away. Probably it could just be
shared_param_list[] from patch 8/8 and shared_refcount (which at that
point won't need to be in struct d3dx_parameter anymore), in addition
to data. You could access the other parameter fields through
shared_param_list[0].
> +static void d3dx_pool_sync_shared_parameter(struct ID3DXEffectPoolImpl *pool, struct d3dx_parameter *param)
> +{
> - unsigned int i, free_entry_index;
> - LONG new_refcount;
> -
> - if (!(param->flags & PARAMETER_FLAG_SHARED) || !pool
> -
>        || is_param_type_sampler(param->type))
> -
>    return;
> -
> - free_entry_index = pool->parameter_count;
> - for (i = 0; i < pool->parameter_count; ++i)
> - {
> -
>    if (!pool->parameters[i]->shared_refcount)
> -
>        free_entry_index = i;
> -
>    else if (is_same_parameter(param, pool->parameters[i]))
> -
>        break;
> - }
> - if (i == pool->parameter_count)
> - {
> -
>    i = free_entry_index;
> -
>    if (i >= pool->table_size)
> -
>    {
> -
>        struct d3dx_parameter ** new_alloc;

No whitespace after "**", please.
> -
>        else
> -
>        {
> -
>            new_size = pool->table_size * 2;
> -
>            new_alloc = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, pool->parameters,
> -
>                    sizeof(*pool->parameters) * new_size);
> -
>            if (!new_alloc)
> -
>            {
> -
>                ERR("Out of memory.\n");
> -
>                return;
> -
>            }
> -
>        }
> -
>        pool->parameters = new_alloc;
> -
>        pool->table_size = new_size;
> -
>    }
> -
>    pool->parameters[i] = HeapAlloc(GetProcessHeap(),
> -
>            HEAP_ZERO_MEMORY, sizeof(*pool->parameters[i]));
> -
>    copy_parameter_structure(param, pool->parameters[i], NULL, FALSE);
> -
>    param_set_data_pointer(pool->parameters[i], (unsigned char *)param->data, FALSE, FALSE);
> -
>    if (i == pool->parameter_count)
> -
>        pool->parameter_count++;
> - }

I don't think this works. HeapReAlloc might move the memory block and
that would break the preexisting effects using the pool (and also the
previous parameters of the new effect). HEAP_REALLOC_IN_PLACE_ONLY
would avoid that but it also makes it easier for the reallocation to
fail.

I see two ways to fix this:
- refresh all the parameters of the effects that are in the pool when
you have to reallocate (shared_param_list[] might help here)
- allocate the memory in chunks, keeping an array (or list) of chunks
which you allocate (but never resize) as needed