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