Re: [PATCH 7/7] d3dx9: Support skip_constants parameter for effect.

[Matteo Bruni]

2017-07-06 13:15 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
> HRESULT d3dx_create_param_eval(struct d3dx9_base_effect *base_effect, void *byte_code,
>          unsigned int byte_code_size, D3DXPARAMETER_TYPE type,
> -        struct d3dx_param_eval **peval, ULONG64 *version_counter) DECLSPEC_HIDDEN;
> +        struct d3dx_param_eval **peval, ULONG64 *version_counter,
> +        const char **skip_constants_names, unsigned int skip_constant_name_count) DECLSPEC_HIDDEN;

I think you can get rid of the "_name" part in those parameters, for brevity.
> +static HRESULT parse_skip_constants_string(char *skip_constants, const char ***names,
> +        unsigned int *name_count)

It's a bit a matter of personal taste but triple pointers look quite
ugly to me. In this case you could e.g. return "names" in place of
HRESULT.
> +{
> +    const char *name;
> +    char *s;
> +    unsigned int size = INITIAL_CONST_NAMES_SIZE;
> +
> +    *names = HeapAlloc(GetProcessHeap(), 0, sizeof(*names) * size);
> +    if (!*names)
> +        return E_OUTOFMEMORY;
> +
> +    *name_count = 0;
> +    s = skip_constants;
> +    while ((name = next_valid_constant_name(&s)))
> +    {
> +        if (*name_count == size)
> +        {
> +            size *= 2;
> +            *names = HeapReAlloc(GetProcessHeap(), 0, *names, sizeof(*names) * size);
> +            if (!*names)
> +                return E_OUTOFMEMORY;

I think you're leaking memory on failure here.
> +        }
> +        (*names)[(*name_count)++] = name;
> +    }
> +    *names = HeapReAlloc(GetProcessHeap(), 0, *names, *name_count * sizeof(*names));
> +    if (!*names)
> +        return E_OUTOFMEMORY;

And here.
> +    for (i = 0; i < skip_constants_name_count; ++i)
> +    {
> +        struct d3dx_parameter *param;
> +
> +        param = get_parameter_by_name(base, NULL, skip_constants_names[i]);
> +        if (param)
> +        {
> +            for (j = 0; j < base->technique_count; ++j)
> +            {
> +                if (is_parameter_used(param, &base->techniques[j]))
> +                {
> +                    WARN("Parameter %s found in skip_constants is used in technique %u.\n",
> +                            debugstr_a(skip_constants_names[i]), j);
> +                    HeapFree(GetProcessHeap(), 0, skip_constants_buffer);
> +                    HeapFree(GetProcessHeap(), 0, skip_constants_names);
> +                    d3dx9_base_effect_cleanup(base);
> +                    return D3DERR_INVALIDCALL;
> +                }
> +            }
> +        }
> +        else
> +        {
> +            TRACE("Parameter %s found in skip_constants not found.\n",
> +                    debugstr_a(skip_constants_names[i]));

This one sounds a bit awkward ("found... not found"). Maybe just
rewrite that as "skip_constants parameter %s not found". You could
apply the same trick to the previous message too.
> +        for (j = 0; j < skip_constant_name_count; ++j)
> +        {
> +            if (!strcmp(cdesc[index].Name, skip_constants_names[j]))

I assume the skip_constants thing isn't used often and, when it is,
very few constants (1-3) are in the list. If that's not the case, we
might want to use an rbtree for the constant names.

[PATCH 7/7] d3dx9: Support skip_constants parameter for effect.

[Paul Gofman]

On 07/11/2017 12:04 AM, Matteo Bruni wrote:
> > +        for (j = 0; j < skip_constant_name_count; ++j)
> > +        {
> > +            if (!strcmp(cdesc[index].Name, skip_constants_names[j]))
> > I assume the skip_constants thing isn't used often and, when it is,
> > very few constants (1-3) are in the list. If that's not the case, we
> > might want to use an rbtree for the constant names.

The only program I know to use it is that one in the bug, and it uses 
~5-6 constants in the list. It is not likely that we can expect much 
more constants somewhere. This loop affects effect creation only. 
Besides, I am not sure rbtree will provide much of optimization for such 
number of constants (and probably its creation effort is also should 
taken into account), the optimization would require some profiling on 
this. I would suggest to leave it as a plain array lookup and not to 
make this part more complicated. If we ever find reasonable to optimize 
effect creation, I suppose there are much wider used parts to start 
from. What do you think?

[PATCH 7/7] d3dx9: Support skip_constants parameter for effect.

[Matteo Bruni]

2017-07-11 10:50 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
> On 07/11/2017 12:04 AM, Matteo Bruni wrote:
> > > +        for (j = 0; j < skip_constant_name_count; ++j)
> > > +        {
> > > +            if (!strcmp(cdesc[index].Name, skip_constants_names[j]))
> >
> > I assume the skip_constants thing isn't used often and, when it is,
> > very few constants (1-3) are in the list. If that's not the case, we
> > might want to use an rbtree for the constant names.
>
> The only program I know to use it is that one in the bug, and it uses ~5-6
> constants in the list.

That is also pretty low.
> It is not likely that we can expect much more
> constants somewhere. This loop affects effect creation only. Besides, I am
> not sure rbtree will provide much of optimization for such number of
> constants (and probably its creation effort is also should taken into
> account), the optimization would require some profiling on this. I would
> suggest to leave it as a plain array lookup and not to make this part more
> complicated. If we ever find reasonable to optimize effect creation, I
> suppose there are much wider used parts to start from. What do you think?

Yep, agreed.