[PATCH v2 1/3] debug.h: cleanup TRACE macros

[Konstantin Kharlamov]

On 28.01.2019 22:10, Alexandre Julliard wrote:
> Konstantin Kharlamov <hi-angel at yandex.ru> writes:
> 
>> On 28.01.2019 19:04, Alexandre Julliard wrote:
>>> Konstantin Kharlamov <hi-angel at yandex.ru> writes:
>>>> While on it, could you please advice on the naming in the 3-rd patch?
>>>> Initially I've used "unlikely" because that's how it's used in
>>>> Mesa. Later then I saw in wine lots of lower-case macros with __
>>>> prefix, so I changed it to "__unlikely". But then there are generic
>>>> macros like "TRACE", and I think of renaming it to "UNLIKELY", which
>>>> is 2 letters shorter, and is upper-case like yet another part of
>>>> macros.
>>>
>>> Considering that the benchmarks don't show an improvement, I don't think
>>> it's worth the trouble.
>>
>> Wait, how so if it does? Besides, even without the benchmark it's
>> obvious that the TRACE appear in so many places that there inevitably
>> would be branch-misses.
>>
>> In addition, this macro can be used for further optimization of
>> performance critical parts of wine code.
>>
>> And what you mean by "the trouble" — it's just a few lines of code
>> that doesn't add any maintainance burden.
> 
> That specific change is clearly a tiny burden, but it's the accumulation
> of such micro-optimizations that ends up being a large burden. That's
> why we require evidence that it actually helps.
> 
> By trying to measure it, you had the right approach, but you need to
> accept the measurement results even if they are not what you
> expected. If you can't demonstrate a clear improvement, then maybe it's
> not as obviously a gain as it may seem at first glance.
> 
> If you want to optimize Wine, I'd suggest picking an app that runs
> slower than it should, and figuring out where the bottlenecks are.
> These are the places that are worth optimizing.
> 

5 millions branch-misses less on average is a clear improvement. Besides 
you need to consider that given TRACE is used a lot in the code, there 
clearly can be workloads where TRACE is executed more often, and hence 
results in slower execution.

I also don't see why would it affect a burden increase in the future. If 
you're referring to using "unlikely" in more places in the code — I can 
assure you, it doesn't make it any less readable, you can see examples 
here 
https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/gallium/drivers/r600/r600_state_common.c#L868