Re: [PATCH 6/6] d3dx9: Merge constant setting for child parameters when possible.

[Paul Gofman]

On 05/29/2017 09:24 PM, Matteo Bruni wrote:
> 2017-05-24 11:46 GMT+02:00 Paul Gofman :
> > -
> >        if (!(const_set->table == current_table && current_start_offset == start_offset
> > -
> >                && const_set->direct_copy == first_const->direct_copy
> > -
> >                && current_data == const_set->param->data
> > -
> >                && (const_set->direct_copy || (first_const->param->type == const_set->param->type
> > -
> >                && first_const->param->class == const_set->param->class
> > -
> >                && first_const->param->columns == const_set->param->columns
> > -
> >                && first_const->param->rows == const_set->param->rows
> > -
> >                && first_const->register_count == const_set->register_count
> > -
> >                && (i == const_tab->const_set_count - 1
> > -
> >                || first_const->param->element_count == const_set->param->element_count)))))
> > -
> >        {
> > -
> >            TRACE("direct_copy %u, i %u, index %u, param %s.%s, current_data %p, const_set->param->data %p, "\
> > -
> >                    "current_start_offset %u, start_offset %u, const_set->table %u, current_table %u.\n",
> > -
> >                    const_set->direct_copy, i, index, debugstr_a(param->name),
> > -
> >                    debugstr_a(const_set->param->name), current_data, const_set->param->data,
> > -
> >                    current_start_offset, start_offset, const_set->table, current_table);
> > -
> >            break;
> > -
> >        }
>
> This looks like a debug trace.
> > -
> >    TRACE("Merging %u child parameters for %s, not merging %u, direct_copy %u.\n", i - index,
> > -
> >            debugstr_a(param->name), const_tab->const_set_count - i, first_const->direct_copy);
>
> I guess it's intentional that you're printing this even when not
> merging anything, in that case the TRACE sounds a bit awkward though.
> Maybe have a separate TRACE for that case?

Both traces are to have a possibility to check from trace log what is 
merged and not "merged" and why, for performance analysis. If you think 
it is more of debug I can remove it, though since it is in 
initialization only I was thinking it does not add too much noise.
> BTW, please format the BOOL with %#x.
> > -
> >    if (i > index + 1)
> > -
> >    {
> > -
> >        first_const->element_count = element_count;
> > -
> >        if (first_const->direct_copy)
> > -
> >        {
> > -
> >            first_const->element_count = 1;
> > -
> >            if (index == start_index
> > -
> >                    && !(param->type == D3DXPT_VOID && param->class == D3DXPC_STRUCT))
> > -
> >            {
> > -
> >                if (param_type_to_table_type(param->type) == PRES_VT_MAX)
> > -
> >                    return D3DERR_INVALIDCALL;
> > -
> >                first_const->param = param;
> > -
> >            }
> > -
> >            first_const->register_count = get_reg_offset(current_table, current_start_offset)
> > -
> >                    - first_const->register_index;
> > -
> >        }
> > -
> >        memmove(&const_tab->const_set[index + 1], &const_tab->const_set[i],
> > -
> >                sizeof(*const_tab->const_set) * (const_tab->const_set_count - i));
> > -
> >        const_tab->const_set_count -= i - index - 1;
> > -
> >    }
>
> Just for my own curiosity, do you think it's significant to merge
> non-direct_copy entries? Not that it hurts anything to have it.

Yes, it is crucial together with the next patches I did not send yet. 
Even in this patchset removing extra const_set entries is beneficial: it 
removes a lot of dirty checks when skipping const_set's from the same 
parameter on CommitChanges, and also initializing the data for inner 
loops less times. In the next patches which is not there yet I:

- Factor out type conversion which can be used for an array of values, 
removing switching for each value being set;

- Introduce a separate path for setting scalar and vectors (which is 
simpler than general matrix case an quite often may be grouped 
effectively even when it is not 'direct_copy');

- Optimize matrix settings loops.

Copying ~100 element array with transpose is quite a common case. 
Merging them in a one const_set and doing that things further is a huge 
optimization actually.