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

[Matteo Bruni]

2017-05-29 20:44 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> On 05/29/2017 09:24 PM, Matteo Bruni wrote:
>>
>> 2017-05-24 11:46 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
>>
>>> +            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.

The first trace feels pretty noisy to me for upstream Wine, do you
think it would be useful in a trace attached on a generic bug to
figure out potential performance improvements?

The "Merging" one is fine, I'm just a tiny bit annoyed by the "Merged
0 child" case.

>> 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.

Cool. Optimizing the transposed matrix case is probably worthy by itself.