Re: Patch: fix lock contention for HASHHDR.mutex

From: Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Patch: fix lock contention for HASHHDR.mutex
Date: 2015-12-15 12:11:19
Message-ID: 20151215151119.1dbd27d1@fujitsu
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello, Tom.

I was exploring this issue further and discovered something strange.

"PROCLOCK hash" and "LOCK hash" are hash tables in shared memory. All
memory for these tables is in fact pre-allocated. But for some reason
these two tables are created (lock.c:394) with init_size =/= max_size.
It causes small overhead on calling memory allocator after hash table
creation and additional locking/unlocking.

I checked all other hash tables created via ShmemInitHash. All of these
tables have init_size == max_size. I suggest to create "PROCLOCK hash"
and "LOCK hash" with init_size == max_size too (see attached patch).
Granted this change doesn't cause any noticeable performance
improvements according to pgbench. Nevertheless it looks like a very
right thing to do which at least doesn't make anything worse.

If this patch seems to be OK next logical step I believe would be to
remove init_size parameter in ShmemInitHash procedure since in practice
it always equals max_size.

On Fri, 11 Dec 2015 10:30:30 -0500
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru> writes:
> > Turns out PostgreSQL can spend a lot of time waiting for a lock in
> > this particular place, especially if you are running PostgreSQL on
> > 60-core server. Which obviously is a pretty bad sign.
> > ...
> > I managed to fix this behaviour by modifying choose_nelem_alloc
> > procedure in dynahash.c (see attached patch).
> TBH, this is just voodoo. I don't know why this change would have
> made any impact on lock acquisition performance, and neither do you,
> and the odds are good that it's pure chance that it changed
> anything. One likely theory is that you managed to shift around
> memory allocations so that something aligned on a cacheline boundary
> when it hadn't before. But, of course, the next patch that changes
> allocations anywhere in shared memory could change that back. There
> are lots of effects like this that appear or disappear based on
> seemingly unrelated code changes when you're measuring edge-case
> performance.
> The patch is not necessarily bad in itself. As time goes by and
> machines get bigger, it can make sense to allocate more memory at a
> time to reduce memory management overhead. But arguing for it on the
> basis that it fixes lock allocation behavior with 60 cores is just
> horsepucky. What you were measuring there was steady-state hash
> table behavior, not the speed of the allocate-some-more-memory code
> path.
> regards, tom lane

Attachment Content-Type Size
lock-contention-v2.patch text/x-patch 1.5 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2015-12-15 12:17:02 Re: [PATCH] Logical decoding support for sequence advances
Previous Message Artur Zakirov 2015-12-15 11:50:06 Re: Allow replication roles to use file access functions