Re: [PATCH] Improve spinlock inline assembly for x86.

On Mon, Jan 18, 2016 at 4:24 PM, Peter Geoghegan <pg@heroku.com> wrote:
> On Mon, Jan 18, 2016 at 2:14 PM, Kevin Grittner <kgrittn@gmail.com> wrote:

>> It's hard to understand quite what you're saying there.  If you're
>> saying that code changes that should be performance neutral can
>> sometimes affect performance because of alignment of code with
>> cache line boundaries -- I absolutely agree; is that an argument
>> against performance testing performance patches?
>
> No, it isn't an argument against performance testing patches like
> this, but I don't think anyone suggested otherwise.

I'm still not sure what argument he *was* making, but I'm glad to
hear it wasn't that.

> Of course every performance related patch should be tested to
> make sure it meets its goals and at acceptable cost,

I argued that it should be tested to ensure that it caused no
regression in a case which has been a problem for some of our
customers (spinlock contention at high concurrency on a machine
with 4 or more NUMA nodes).  We have been able to solve those
problems, but it has been a fussy business -- sometimes we have
tweaked spinlock code and sometimes that has not worked but an OS
upgrade has.  I am quite unconvinced about whether the change will
help, hurt, or have no impact on these problems; I'm only arguing
for testing to find out.

> but I don't think that Andreas' patch is necessarily a
> performance patch. There can be value in removing superfluous
> code; doing so sometimes clarifies intent and understanding.

Well, that's why I said I would be satisfied with a neutral
benchmark result -- when there is a tie, the shorter, simpler code
should generally win.  I'm not really sure what there was in what I
said to argue about; since that I've just been trying figure that
out.  If we all agree, let's let it drop.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company