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

On Mon, Jan 18, 2016 at 3:47 PM, Andres Freund <andres@anarazel.de> wrote:
> On January 18, 2016 10:42:42 PM GMT+01:00, Kevin Grittner <kgrittn@gmail.com> wrote:

>> I took a look at this and agree that the shorter, simpler code
>> proposed in this patch should make no *logical* difference, and
>> looks like it *should* have a neutral or beneficial affect; but
>> performance tuning in general, and spinlock performance in
>> particular, is full of surprises.  We have seen customers suffer
>> poor scaling on their brand new monster machines because of the
>> interaction between NUMA scheduling and our spinlock
>> implementation, and seen those problems go away with an upgrade
>> from pre-3.8 to post-3.8 kernels.  I would be hesitant to accept
>> this change without seeing a benchmark on a large NUMA machine with
>> 4 or more memory nodes, on Linux kernels both before and after 3.8,
>> to make sure that the effects are at least neutral.
>
> Unconvinced.

Unconvinced that we should do performance testing on a proposed
performance patch before accepting it, that the changes in NUMA
scheduling in the Linux 3.8 kernel have a major effect on how well
our code performs at high concurrency on NUMA machines with a lot
of memory nodes, that patches to improve performance sometimes
cause surprising regressions, that the results will come out any
particular way, or that the difference will be out of the noise?
Personally I'm only convinced on the first three of those.

> You get just as much churn by changing code elsewhere, which
> often causes code movement and alignment changes.

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?

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