On 29.01.2019 12:08, Alexandre Julliard wrote:
Konstantin Kharlamov hi-angel@yandex.ru writes:
On 29.01.2019 11:01, Alexandre Julliard wrote:
Konstantin Kharlamov hi-angel@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.