[PATCH 3/3] debug.h: hint a compiler: TRACE is not executed in common usage

[Konstantin Kharlamov]

On 29.01.2019 12:08, Alexandre Julliard wrote:
> Konstantin Kharlamov <hi-angel at yandex.ru> writes:
> 
>> On 29.01.2019 11:01, Alexandre Julliard wrote:
>>> Konstantin Kharlamov <hi-angel at yandex.ru> writes:
>>>
>>>> On 29.01.2019 01:17, Marvin wrote:
>>>>> Thank you for your contribution to Wine!
>>>>>
>>>>> This is an automated notification to let you know that your patch has
>>>>> been reviewed and its status set to "Rejected".
>>>>>
>>>>> This means that the patch has been rejected by a reviewer. You should
>>>>> have received a mail explaining why it was rejected. You need to fix
>>>>> the issue and resend the patch, or if you are convinced that your
>>>>> patch is good as is, you should reply to the rejection message with
>>>>> your counterarguments.
>>>>>
>>>>> If you do not understand the reason for this status, disagree with our
>>>>> assessment, or are simply not sure how to proceed next, please ask for
>>>>> clarification by replying to this email.
>>>>
>>>> Hi, I don't understand, why the 3-rd patch was marked rejected? AFAIK
>>>> it's being discussed.
>>>
>>> I explained that such micro-optimizations won't be accepted, unless
>>> there is clear evidence that they make a difference. Your numbers are
>>> not convincing enough I'm afraid.
>>
>> But why? It's still an optimization, and as being a burden it is very tiny. It is definitely smaller than the burden that my rejected refactoring was removing, why can't it be accepted?
>>
>> Okay, if we're into this discussion: can you please formalize, what burden does it add from your point of view?
> 
> The default position is to keep the code as is; if you want to change
> it, it's up to you to demonstrate that the change is worth it. That's
> true for every submitted patch; it's even more true when making changes
> to things like debug.h that are used everywhere, where even a small
> change can have large consequences.

Fair enough. Then, let's break down pros and cons for the patch mentioned in discussion:

pros:
	1. it improves general execution of the code. TRACE is the *most* often evaluated branch in wine code, because it is used everywhere in the codebase. Benchmark did show a small improvement, but even without it it's a clear and easy to achieve optimization.
	2. it adds a macro that can be used for further optimization of performance critical parts of the code, such as various generic algorithms, heap scheduling, and what not.

cons:
	1. it adds a small burden

The abstract "small burden" is added by any code. You can reject virtually any patch that increases a line count by stating that it adds a small code burden.

In this particular case the "burden" resides in a part of codebase that rarely being looked upon, in addition it's just a line being wrapped into "__unlikely" which is hardly would take an effort for anyone to read. Especially if I add the "unlikely" declaration just above the place where it's used.

Codeweaver's customers would surely appreciate if their software would work a bit faster. In particular on those Macs with slow CPUs, my gf has one. And then I heard, Codeweavers are trying to extend their market to Android, right? Which has even slower CPUs.

Let's just let this improvement in, please.