Re: [v2 6/6] d3dx9: Support parameters sharing in effect.

[Matteo Bruni]

2017-04-24 13:36 GMT+02:00 Paul Gofman :
> +struct d3dx_shared_data
> +{
> - void *data;
> - LONG refcount;
> - struct d3dx_parameter **parameters;
> - unsigned int table_size;
>
> +};

A bit nitpicky but generally for those dynamic arrays we call
"refcount" "count" and "table_size" "size".
> - new_refcount = InterlockedIncrement(&pool->shared_data[i].refcount);

I think you don't need the Interlocked functions for this count given
that adding / removing multiple effects to a pool is expected to be
racy.
> - if (new_refcount >= pool->shared_data[i].table_size)
> - {
> -
>    if (!pool->shared_data[i].table_size)
> -
>    {
> -
>        new_size = INITIAL_SHARED_DATA_TABLE_SIZE;
> -
>        pool->shared_data[i].parameters = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> -
>                sizeof(*pool->shared_data[i].parameters) * INITIAL_SHARED_DATA_TABLE_SIZE);
> -
>    }
> -
>    else
> -
>    {
> -
>        new_size = pool->shared_data[i].table_size * 2;
> -
>        pool->shared_data[i].parameters = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> -
>                pool->shared_data[i].parameters,
> -
>                sizeof(*pool->shared_data[i].parameters) * new_size);
> -
>    }
> -
>    pool->shared_data[i].table_size = new_size;
> - }
> -
> - param->shared_data = &pool->shared_data[i];
> - pool->shared_data[i].parameters[new_refcount - 1] = param;
> -
> - TRACE("name %s, parameter idx %u, new refcount %u.\n", param->name, i,
> -
>        new_refcount);
> - return;
>
> +}

Unnecessary return.
> +static BOOL param_zero_data_func(void *dummy, struct d3dx_parameter *param)
> +{
> - param->data = NULL;
> - return FALSE;
>
> +}
> -
>
> +static void d3dx_pool_release_shared_parameter(struct d3dx_parameter *param)
> +{
> - LONG new_refcount;
> -
> - if (!(param->flags & PARAMETER_FLAG_SHARED) || !param->shared_data)
> -
>    return;

No need to check for the PARAMETER_FLAG_SHARED flag. Maybe it makes
sense to assert that the flag is there if param->shared_data is not
NULL.
> static void free_effect_pool(struct d3dx_effect_pool *pool)
>  {
> - unsigned int i;
> -
> - for (i = 0; i < pool->table_size; ++i)
> - {
> -
>    if (pool->shared_data[i].refcount)
> -
>        ERR("Releasing pool with referenced parameters, shared data is not freed.\n");
> - }

This can be triggered by an application so it should be a WARN().
BTW, this probably needs a test. On pool destruction, the effects
still using it might be "unshared" i.e. data from the pool might be
moved back to the effects. Or maybe the effects still using the pool
are simply broken and accessing parameter data after the pool is
destroyed crashes.

[v2 6/6] d3dx9: Support parameters sharing in effect.

[Paul Gofman]

On 04/25/2017 06:35 PM, Matteo Bruni wrote:
> > +static BOOL param_zero_data_func(void *dummy, struct d3dx_parameter *param)
> > +{
> > - param->data = NULL;
> > - return FALSE;
> >
> > +}
> > -
> >
> > +static void d3dx_pool_release_shared_parameter(struct d3dx_parameter *param)
> > +{
> > - LONG new_refcount;
> > -
> > - if (!(param->flags & PARAMETER_FLAG_SHARED) || !param->shared_data)
> > -
> >    return;
>
> No need to check for the PARAMETER_FLAG_SHARED flag. Maybe it makes
> sense to assert that the flag is there if param->shared_data is not
> NULL.

I need to check this flag because shared_data pointer is "unioned" with 
reference_param, and removing the check should break freeing of 
non-shared parameters with non-zero referenced_param. Checking just a 
flag won't do either, as actually the parameter marked as shared could 
have nothing to be shared with.
> > static void free_effect_pool(struct d3dx_effect_pool *pool)
> >   {
> > - unsigned int i;
> > -
> > - for (i = 0; i < pool->table_size; ++i)
> > - {
> > -
> >    if (pool->shared_data[i].refcount)
> > -
> >        ERR("Releasing pool with referenced parameters, shared data is not freed.\n");
> > - }
>
> This can be triggered by an application so it should be a WARN().
> BTW, this probably needs a test. On pool destruction, the effects
> still using it might be "unshared" i.e. data from the pool might be
> moved back to the effects. Or maybe the effects still using the pool
> are simply broken and accessing parameter data after the pool is
> destroyed crashes.

Yes, sure, there is one of the marginal cases to be tested more 
(probably along with more precise understanding on how shared shader 
parameters setting works when sharing non-NULL and NULL shader). Until 
then maybe we could leave something more severe than WARN here, as if we 
reach this point it most likely mean that something is wrong and it 
would be nice to have an indication right away.

[v2 6/6] d3dx9: Support parameters sharing in effect.

[Matteo Bruni]

2017-04-25 18:03 GMT+02:00 Paul Gofman :
> On 04/25/2017 06:35 PM, Matteo Bruni wrote:
> > >     +static BOOL param_zero_data_func(void *dummy, struct
> > >
> > > d3dx_parameter *param)
> > > +{
> > > - param->data = NULL;
> > > - return FALSE;
> > >
> > > +}
> > > -
> > >
> > > +static void d3dx_pool_release_shared_parameter(struct d3dx_parameter
> > > *param)
> > > +{
> > > - LONG new_refcount;
> > > -
> > > - if (!(param->flags & PARAMETER_FLAG_SHARED) || !param->shared_data)
> > > -
> > >    return;
> >
> > No need to check for the PARAMETER_FLAG_SHARED flag. Maybe it makes
> > sense to assert that the flag is there if param->shared_data is not
> > NULL.
>
> I need to check this flag because shared_data pointer is "unioned" with
> reference_param, and removing the check should break freeing of non-shared
> parameters with non-zero referenced_param. Checking just a flag won't do
> either, as actually the parameter marked as shared could have nothing to be
> shared with.

Good point.
> > > static void free_effect_pool(struct d3dx_effect_pool *pool)
> > >   {
> > > - unsigned int i;
> > > -
> > > - for (i = 0; i < pool->table_size; ++i)
> > > - {
> > > -
> > >    if (pool->shared_data[i].refcount)
> > > -
> > >        ERR("Releasing pool with referenced parameters, shared data
> > >
> > > is not freed.\n");
> > > - }
> >
> > This can be triggered by an application so it should be a WARN().
> > BTW, this probably needs a test. On pool destruction, the effects
> > still using it might be "unshared" i.e. data from the pool might be
> > moved back to the effects. Or maybe the effects still using the pool
> > are simply broken and accessing parameter data after the pool is
> > destroyed crashes.
>
> Yes, sure, there is one of the marginal cases to be tested more (probably
> along with more precise understanding on how shared shader parameters
> setting works when sharing non-NULL and NULL shader). Until then maybe we
> could leave something more severe than WARN here, as if we reach this point
> it most likely mean that something is wrong and it would be nice to have an
> indication right away.

I think we want to know what happens before we go ahead with the code above.

[v2 6/6] d3dx9: Support parameters sharing in effect.

[Paul Gofman]

On 04/25/2017 07:09 PM, Matteo Bruni wrote:
> > > > static void free_effect_pool(struct d3dx_effect_pool *pool)
> > > >    {
> > > > - unsigned int i;
> > > > -
> > > > - for (i = 0; i < pool->table_size; ++i)
> > > > - {
> > > > -
> > > >    if (pool->shared_data[i].refcount)
> > > > -
> > > >        ERR("Releasing pool with referenced parameters, shared data
> > > >
> > > > is not freed.\n");
> > > > - }
> > >
> > > This can be triggered by an application so it should be a WARN().
> > > BTW, this probably needs a test. On pool destruction, the effects
> > > still using it might be "unshared" i.e. data from the pool might be
> > > moved back to the effects. Or maybe the effects still using the pool
> > > are simply broken and accessing parameter data after the pool is
> > > destroyed crashes.
> >
> > Yes, sure, there is one of the marginal cases to be tested more (probably
> > along with more precise understanding on how shared shader parameters
> > setting works when sharing non-NULL and NULL shader). Until then maybe we
> > could leave something more severe than WARN here, as if we reach this point
> > it most likely mean that something is wrong and it would be nice to have an
> > indication right away.
>
> I think we want to know what happens before we go ahead with the code above.

I just tested, manually releasing pool before effect just crash when I 
use native d3dx dll. Please see attached patch. I have to do release 
twice as it is referenced from 2 effects. If I release pool just once, 
it crashes in freeing effect.

[v2 6/6] d3dx9: Support parameters sharing in effect.

[Matteo Bruni]

2017-04-25 18:24 GMT+02:00 Paul Gofman :
> On 04/25/2017 07:09 PM, Matteo Bruni wrote:
> > > > > static void free_effect_pool(struct d3dx_effect_pool *pool)
> > > > >    {
> > > > > - unsigned int i;
> > > > > -
> > > > > - for (i = 0; i < pool->table_size; ++i)
> > > > > - {
> > > > > -
> > > > >    if (pool->shared_data[i].refcount)
> > > > > -
> > > > >        ERR("Releasing pool with referenced parameters, shared
> > > > >
> > > > > data
> > > > > is not freed.\n");
> > > > > - }
> > > >
> > > > This can be triggered by an application so it should be a WARN().
> > > > BTW, this probably needs a test. On pool destruction, the effects
> > > > still using it might be "unshared" i.e. data from the pool might be
> > > > moved back to the effects. Or maybe the effects still using the pool
> > > > are simply broken and accessing parameter data after the pool is
> > > > destroyed crashes.
> > >
> > > Yes, sure, there is one of the marginal cases to be tested more (probably
> > > along with more precise understanding on how shared shader parameters
> > > setting works when sharing non-NULL and NULL shader). Until then maybe we
> > > could leave something more severe than WARN here, as if we reach this
> > > point
> > > it most likely mean that something is wrong and it would be nice to have
> > > an
> > > indication right away.
> >
> > I think we want to know what happens before we go ahead with the code
> > above.
>
> I just tested, manually releasing pool before effect just crash when I use
> native d3dx dll. Please see attached patch. I have to do release twice as it
> is referenced from 2 effects. If I release pool just once, it crashes in
> freeing effect.

Excellent. Adding the test in an if (0) {} + a comment would probably
be nice for documentation purposes. For the implementation I'd go with
WARN + releasing shared data.

[v2 6/6] d3dx9: Support parameters sharing in effect.

[Paul Gofman]

On 04/25/2017 07:34 PM, Matteo Bruni wrote:
> 2017-04-25 18:24 GMT+02:00 Paul Gofman :
> > On 04/25/2017 07:09 PM, Matteo Bruni wrote:
> > > > > > static void free_effect_pool(struct d3dx_effect_pool *pool)
> > > > > > {
> > > > > > - unsigned int i;
> > > > > > -
> > > > > > - for (i = 0; i < pool->table_size; ++i)
> > > > > > - {
> > > > > > -
> > > > > >    if (pool->shared_data[i].refcount)
> > > > > > -
> > > > > >        ERR("Releasing pool with referenced parameters, shared
> > > > > >
> > > > > > data
> > > > > > is not freed.\n");
> > > > > > - }
> > > > >
> > > > > This can be triggered by an application so it should be a WARN().
> > > > > BTW, this probably needs a test. On pool destruction, the effects
> > > > > still using it might be "unshared" i.e. data from the pool might be
> > > > > moved back to the effects. Or maybe the effects still using the pool
> > > > > are simply broken and accessing parameter data after the pool is
> > > > > destroyed crashes.
> > > >
> > > > Yes, sure, there is one of the marginal cases to be tested more (probably
> > > > along with more precise understanding on how shared shader parameters
> > > > setting works when sharing non-NULL and NULL shader). Until then maybe we
> > > > could leave something more severe than WARN here, as if we reach this
> > > > point
> > > > it most likely mean that something is wrong and it would be nice to have
> > > > an
> > > > indication right away.
> > >
> > > I think we want to know what happens before we go ahead with the code
> > > above.
> >
> > I just tested, manually releasing pool before effect just crash when I use
> > native d3dx dll. Please see attached patch. I have to do release twice as it
> > is referenced from 2 effects. If I release pool just once, it crashes in
> > freeing effect.
>
> Excellent. Adding the test in an if (0) {} + a comment would probably
> be nice for documentation purposes. For the implementation I'd go with
> WARN + releasing shared data.

Could you please advise how to best add it: just as this patch in if(0) 
{}, or a separate test?

[v2 6/6] d3dx9: Support parameters sharing in effect.

[Matteo Bruni]

2017-04-25 18:46 GMT+02:00 Paul Gofman :
> Could you please advise how to best add it: just as this patch in if(0) {},
> or a separate test?

I prefer the former i.e. no separate test. Thanks!