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

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

[Paul Gofman]

On 04/21/2017 08:11 PM, Matteo Bruni wrote:
> 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].

This should be possible somehow but this is not quite straightforward. 
Imagine the case we are creating 3 effects with shared parameter, then 
remove the first one, which effectively holds the data shared between 
the three. I will need to move the data to the next parameter then. 
Right now it seems to me I can just copy the data when removing the 
first parameter (though just memcpy won't do, there might be shaders or 
texture pointers), but need to look at it more to make sure I am not 
missing something.
> > +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 am not sure what is exactly wrong here? I am not referencing the 
(re)allocated memory here. pool->parameters[i] is a pointer, and 
parameter receives the pointers's value (pool->parameters[i]), not 
pointer to it. When pool->parameters is reallocated to the new place, 
nothing bad happens, as pointers to parameters remain the same. The 
problem comes if I try to use &pool->parameters[i] somewhere, but I 
don't. Am I missing something?
> 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