[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: removing useless if-before-free tests
From: |
Jim Meyering |
Subject: |
Re: removing useless if-before-free tests |
Date: |
Tue, 19 Feb 2008 09:45:47 +0100 |
Bruno Haible <address@hidden> wrote:
> Jim Meyering wrote:
>> I agree those should go, along with all of the others in tests/*.c.
>
> I agree about the tests. Tests can afford small slowdowns.
>
>> I'd like to avoid spending time looking for ways to micro-optimize.
>> How about if I remove them all, and then restore-with-justification
>> the ones for which you think there is a significant performance gain?
>> Then, at least, there will be some indication to future reviewers
>> why we've kept a seemingly redundant test.
>
> No, I heavily disagree with such an approach. It should be the other way
> around: Remove the 'if' only with a written justification to which the
> author agrees.
??? No one is proposing to modify your files without permission.
> - Saying "I will break your performance, because I don't have much time,
> then _you_ please fix up what I broke" is simply unfriendly.
Yes, saying that would be unfriendly.
However, I did not say or imply any such thing, and wonder why
you try to attribute such unpleasantness to me.
A difference of a few instructions "breaks performance"???
Isn't that perhaps just a tiny exaggeration?
> I don't have much more time than you have.
You have misread. I offered to make both changes.
However, since I don't believe in making redundancy-adding changes
without evidence, and don't want to search for micro-optimizations,
I proposed to document whatever redundant tests you preferred to
retain, by re-adding them. I am prepared to wait until you have
found time to justify whatever you want. Of course, if you want
to make the changes yourself, that's even better.
> - Code that Paul, I, or Eric wrote has a lot of care and thought in it.
> It is wrong to assume that because there is no comment at a certain place,
> the author did not think about it.
You're reading way too much into my words.
There was no such assumption on my part.
However, I *do* assume that few people write
if (p)
free (p);
to save a few instructions over a bare "free (p);", when "p" is often
NULL. Here, we are all well aware of gnulib's "free" module, so in code
that does not depend on that module, I think it's only fair to assume that
the redundant "if" is to avoid the risk of a nonconforming free function.
Personally, I would not add an extra test like that unless I'd
profiled the code in question and found a significant improvement.
And if I'd taken the time to profile and add the test, I would have
felt obliged to add a comment justifying code what otherwise might seem
redundant.
> - (A less relevant argument:) Among the occurrences of the pattern in lib/*.c
> that I briefly looked at, the majority appears to be in the case where NULL
> is frequently used, i.e. where the 'if' should be kept for performance.
I'm surprised that you would come to such a conclusion.
I've been taught that "preoptimization is the root of all evil" (Knuth).
If you've already made some performance measurements to support that
claim, please let us know.
> A different approach would be if you said: "I found some useless 'if's in
> these files - Bruno -, in these files - Eric -, in these files - Simon -,
> etc. Please look which of them you can remove." This way, you're friendly
> towards everyone, you don't break code that is not yours, and you don't
> need to spend much time.
I find it hard to understand your arguing for a more friendly attitude
when you yourself exhibit such a confrontational and abrasive manner.
-------------------
Try to see this from my perspective:
I saw this as a simple, mechanical, obviously-correct, and
non-controversial change. I wanted to make the global change
efficiently (spending little of my time), and didn't anticipate
any objection. That was my mind-set when I started this thread.
Imagine my surprise when you object, citing the potential for
this to be a "performance-breaking" change, and then consider
your abrasive manner and misrepresentations in the message to
which I am now replying. I have to confess I'm taken aback.
You've made many fine contributions on the technical front.
Please don't spoil the atmosphere here.
Jim