[RFC] Performance improvements

[Martin Schwenke]

Hi Swen,

On Fri, 13 Jul 2018 12:56:56 +0200, Swen Schillig <swen at vnet.ibm.com>
wrote:

> > The destructor does not set queue->destroyed = true at all.  It does
> > something slightly different, please re-read.   
> 
> Boaah Martin, please don't tell me you want me to find the distinction
> between queue->destroyed and *queue->destroyed.
> At the end of the day a value is set which is not read anywhere in the
> code and it's parent struct (and memory representation) is
> free'd/invalidated in that very same job.

Yes, but queue->destroyed is a pointer to a bool, which is somewhere
else.  So setting *queue->destroyed sets whatever is pointed to and
freeing the structure doesn't free or invalidate that bool.  So when
you say that whatever has been assigned is freed immediately, I feel a
distinct lack of confidence and I see a commit message that isn't
useful to someone looking at it in the future.

It is true that now this element isn't assigned to or otherwise
referenced outside the destructor.  So, given that setting that bool
was the only useful thing the destructor did, then pointing to when the
last use was removed seems like a helpful thing to do.

This patch is buried in amongst some patches for performance
improvements. So, without a pointer to when it became unused, the first
thing I need to do is spend time looking at the previous patches in your
set to see if one of those has caused it to be unused.  Then, when I
find that isn't the case, I need to do the digging to understand when
it became unused so that I am confident that the patch is correct.

If you provide the pointer for me (and post the patch as a
separate clean-up that removes unused code) then the situation is
totally clear. Then I take a look at that commit, search for
"destroyed" in the current code, say "huh, that's a nice simple clean
up", reply with a Reviewed-by: tag and the whole process is much
simpler.

> >  The destructor was once useful. 
> >  All I'm asking you to do is mention in the commit message
> > when it stopped being useful.   
> 
> Hmm, I thought it is enough to see that it is useless now.
> Anyhow, not really sure about its usefulness in the past but I believe
> it definitely lost its "purpose" with David's patch to make
> queue_io_read reentrant.
> 
> commit e097b7f8ff1a9992de1d11474dac4969e30cd679
> Author: David Disseldorp <ddiss at suse.de>
> Date:   Sun Jul 31 03:14:54 2011 +0200
> 
>     io: Make queue_io_read() safe for reentry
> 
> >  I'm not arguing against removal of the
> > destructor.  I just don't want to have to go on archaeological dig
> > every time I look at this code.  If your commit message points to the
> > right point in the past then I know what I need to know and you've
> > saved me some work.  

> So am I getting you right in saying that if I'm adding a comment with
> the above reference would convince you to accept his one ?

Of course!  You might say something about it removing the last use of
the "destroyed" element, which seems to have been the whole point of
the destructor.

> See....you got your victory here...now be a good diplomat and give one
> to me as well :-)

There are no victories to be had here.  There are only clean code
changes with clean commit messages...

peace & happiness,
martin