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

[Matteo Bruni]

2017-05-29 20:44 GMT+02:00 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.

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.