Re: measuring spinning

On Thu, Jun 14, 2012 at 2:39 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jan 11, 2012 at 8:48 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I've had cause, a few times this development cycle, to want to measure
>> the amount of spinning on each lwlock in the system.  To that end,
>> I've found the attached patch useful.  Note that if you don't define
>> LWLOCK_STATS, this changes nothing except that the return value from
>> s_lock becomes int rather than void.  If you do define LWLOCK_STATS,
>> then LWLockAcquire() counts the number of pg_usleep() calls that are
>> required to acquire each LWLock, in addition to the other statistics.
>> Since this has come up for me a few times now, I'd like to proposing
>> including it in core.
>
> Well, this fell through the cracks, because I forgot to add it to the
> January CommitFest.  Here it is again, rebased.

This applies and builds cleanly and passes make check (under enable-cassert).

Not test or docs are needed for a patch of this nature.

It does what it says, and we want it.

I wondered if the change in the return signature of s_lock would have
an affect on performance. So I've run a series of pgbench -T 30 -P
-c8 -j8, at a scale of 30 which fits in shared_buffers, using an
Amazon c1.xlarge
(8 cores). I ran both HEAD, and HEAD+patch (without LWLOCK_STATS in
both cases), in random ordering. The patch was 0.37% slower, average
298483 selects per second patched to 299582 HEAD. The difference is
probably real (p value 0.042, one sided.) but is also pretty much
negligible and could just be due to where the executable code falls in
the cache lines which could move around with other changes to the
code.

Two suggestions:

In your original email you say "number of pg_usleep() calls that are
required to acquire each LWLock", but nothing in the code says this.
Just reading lwlock.c I would naively assume it is reporting the
number of TAS spins, not the number of spin-delays (and in fact that
is what I did assume until I read your email more carefully). A
comment somewhere in lwlock.c would be helpful.

Also in lwlock.c,

if (sh_acquire_counts[i] || ex_acquire_counts[i] ||
block_counts[i] || spin_counts[i])

I don't think we can have spins (or blocks, for that matter) unless we
have some acquires to have caused them, so the last two tests in that
line seem to be noise.

Since my suggestions are minor, should I go ahead and mark this ready
for committer?

Thanks,

Jeff