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

[Paul Gofman]

On 05/30/2017 02:03 AM, Matteo Bruni wrote:
> 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?

I will remove it. Anyway, I can recover the same information from the 
other present TRACE data, just harder. I don't think this particular 
message alone worth putting as a "performance debugging" patch. If to do 
some performance analysis remotely, I would start from time measurements 
dumping in FIXMEs, though I don't have any more or less universal patch 
for that now apart from timing BeginPass / CommitChanges. I did inner 
functions timing at some points but adding that for all possible 
functions of interest together is not feasible as it does biases the 
outer functions measurements. If to think of a general patch it probably 
should have switches of what functions to profile, or just rotate that 
switches on the fly.
> The "Merging" one is fine, I'm just a tiny bit annoyed by the "Merged
> 0 child" case.

BTW merging "0" technically never happens, merging "1" means that it 
actually merging nothing. I will add another TRACE message for this 
case. Ironically I had that before some last edits, but considered a 
single unified TRACE message nicer than more code branching and 
different TRACEs.