Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1aOQ2C-0007o7-8q for pgsql-hackers@arkaria.postgresql.org; Wed, 27 Jan 2016 13:21:20 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84) (envelope-from ) id 1aOQ2B-0004S9-JF for pgsql-hackers@arkaria.postgresql.org; Wed, 27 Jan 2016 13:21:19 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84) (envelope-from ) id 1aOQ0o-0001hN-Uf for pgsql-hackers@postgresql.org; Wed, 27 Jan 2016 13:19:55 +0000 Received: from newmail.postgrespro.ru ([93.174.131.138] helo=mail.postgrespro.ru) by makus.postgresql.org with esmtp (Exim 4.84) (envelope-from ) id 1aOQ0g-0001T1-VF for pgsql-hackers@postgresql.org; Wed, 27 Jan 2016 13:19:53 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.postgrespro.ru (Postfix) with ESMTP id D57C221C5BF4; Wed, 27 Jan 2016 16:19:44 +0300 (MSK) Received: from mail.postgrespro.ru ([127.0.0.1]) by localhost (mail.postgrespro.ru [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jcD0fRojPvsk; Wed, 27 Jan 2016 16:19:43 +0300 (MSK) Received: from [192.168.27.79] (unknown [192.168.27.1]) by mail.postgrespro.ru (Postfix) with ESMTPSA id 7763B21C5B80; Wed, 27 Jan 2016 16:19:43 +0300 (MSK) Subject: Re: Patch: fix lock contention for HASHHDR.mutex To: Aleksander Alekseev References: <20151211170001.78ded9d7@fujitsu> <20151215122513.GC17724@alap3.anarazel.de> <20151217151418.GB2224@awork2.anarazel.de> <20151217190342.07e4533a@fujitsu> <56740E41.4010708@sigaev.ru> <20151222183953.771cb58b@fujitsu> <20151228120112.6a847dfd@fujitsu> <20151229184851.1bb7d1bd@fujitsu> <20151230160137.034371e0@fujitsu> <56A0E536.8020504@postgrespro.ru> <20160122134837.788ecf83@fujitsu> Cc: pgsql-hackers From: Anastasia Lubennikova Message-ID: <56A8C3EF.7030604@postgrespro.ru> Date: Wed, 27 Jan 2016 16:19:43 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160122134837.788ecf83@fujitsu> Content-Type: multipart/alternative; boundary="------------090806020702080007000706" X-Pg-Spam-Score: -1.9 (-) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgsql-hackers Precedence: bulk Sender: pgsql-hackers-owner@postgresql.org This is a multi-part message in MIME format. --------------090806020702080007000706 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 22.01.2016 13:48, Aleksander Alekseev: >> Then, this thread became too tangled. I think it's worth to write a >> new message with the patch, the test script, some results and brief >> overview of how does it really works. It will make following review >> much easier. > Sure. > > HASHHDR represents a hash table. It could be usual or partitioned. > Partitioned table is stored in a shared memory and accessed by multiple > processes simultaneously. To prevent data corruption hash table is > partitioned and each process has to acquire a lock for a corresponding > partition before accessing data in it. Number of partition is determine > by lower bits of key's hash value. Most tricky part is --- dynahash > knows nothing about these locks, all locking is done on calling side. > > Since shared memory is pre-allocated and can't grow to allocate memory > in a hash table we use freeList. Also we use nentries field to count > current number of elements in a hash table. Since hash table is used by > multiple processes these fields are protected by a spinlock. Which as > it turned out could cause lock contention and create a bottleneck. > > After trying a few approaches I discovered that using one spinlock per > partition works quite well. Here are last benchmark results: > > http://www.postgresql.org/message-id/20151229184851.1bb7d1bd@fujitsu > > Note that "no locks" solution cant be used because it doesn't guarantee > that all available memory will be used in some corner cases. > > You can find a few more details and a test script in the first message > of this thread. If you have any other questions regarding this patch > please don't hesitate to ask. InitLocks /* * Compute init/max size to request for lock hashtables. Note these * calculations must agree with LockShmemSize! */ This comment certainly requires some changes. BTW, could you explain why init_table_size was two times less than max_table_size? Although, I don't see any problems with your changes. - hctl->nentries = 0; - hctl->freeList = NULL; Why did you delete these two lines? I wonder if you should rewrite them instead? + * This particular number of partitions significantly reduces lock contention + * in partitioned hash tables, almost if partitioned tables didn't use any + * locking at all As far as I understood, this number was obtained experimentally? Maybe you should note that in the comment. And the last, but not least. + nelem_alloc = ((int) nelem) / partitions_number; + if (nelem_alloc == 0) + nelem_alloc = 1; + + for (i = 0; i < partitions_number; i++) + if (!element_alloc(hashp, nelem_alloc, i)) + ereport(ERROR, + (errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory"))); It looks like you forgot to round the result of the division. For example, if you have nelem=25 and partitions_number=6. 25 / 6 = 4. And then you allocate 24 nelems, while 1 is lost. What about productivity, I did a test on my notebook. Patched version shows small performance improvement. pgbench -j 1 -c 1 -f pgbench.sql -T 300 postgres base patched 127tps 145tps pgbench -j 8 -c 8 -f pgbench.sql -T 300 postgres base patched 247tps 270tps But I haven't any big machine to perform tests, where the patch is supposed to make significant changes. So I would rely on your and the other reviewers results. Except mentioned notes, I suppose the patch is good enough to pass it to a reviewer. -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company --------------090806020702080007000706 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 8bit

22.01.2016 13:48, Aleksander Alekseev:
Then, this thread became too tangled. I think it's worth to write a
new message with the patch, the test script, some results and brief
overview of how does it really works. It will make following review
much easier.

Sure.

HASHHDR represents a hash table. It could be usual or partitioned.
Partitioned table is stored in a shared memory and accessed by multiple
processes simultaneously. To prevent data corruption hash table is
partitioned and each process has to acquire a lock for a corresponding
partition before accessing data in it. Number of partition is determine
by lower bits of key's hash value. Most tricky part is --- dynahash
knows nothing about these locks, all locking is done on calling side.

Since shared memory is pre-allocated and can't grow to allocate memory
in a hash table we use freeList. Also we use nentries field to count
current number of elements in a hash table. Since hash table is used by
multiple processes these fields are protected by a spinlock. Which as
it turned out could cause lock contention and create a bottleneck.

After trying a few approaches I discovered that using one spinlock per
partition works quite well. Here are last benchmark results:

http://www.postgresql.org/message-id/20151229184851.1bb7d1bd@fujitsu

Note that "no locks" solution cant be used because it doesn't guarantee
that all available memory will be used in some corner cases.

You can find a few more details and a test script in the first message
of this thread. If you have any other questions regarding this patch
please don't hesitate to ask.
InitLocks
    /*
     * Compute init/max size to request for lock hashtables.  Note these
     * calculations must agree with LockShmemSize!
     */

This comment certainly requires some changes.
BTW, could you explain why init_table_size was two times less than max_table_size?
Although, I don't see any problems with your changes.


-    hctl->nentries = 0;
-    hctl->freeList = NULL;

Why did you delete these two lines? I wonder if you should rewrite them instead?

+ * This particular number of partitions significantly reduces lock contention
+ * in partitioned hash tables, almost if partitioned tables didn't use any
+ * locking at all

As far as I understood, this number was obtained experimentally? Maybe you should note that in the comment.


And the last, but not least.

+        nelem_alloc = ((int) nelem) / partitions_number;
+        if (nelem_alloc == 0)
+            nelem_alloc = 1;
+
+        for (i = 0; i < partitions_number; i++)
+            if (!element_alloc(hashp, nelem_alloc, i))
+                ereport(ERROR,
+                        (errcode(ERRCODE_OUT_OF_MEMORY),
+                         errmsg("out of memory")));


It looks like you forgot to round the result of the division.
For example, if you have nelem=25 and partitions_number=6.
25 / 6 = 4. And then you allocate 24 nelems, while 1 is lost.

What about productivity, I did a test on my notebook. Patched version shows small performance improvement.

pgbench -j 1 -c 1 -f pgbench.sql -T 300 postgres
base patched
127tps  145tps
pgbench -j 8 -c 8 -f pgbench.sql -T 300 postgres
base patched
247tps  270tps

But I haven't any big machine to perform tests, where the patch is supposed to make significant changes.
So I would rely on your and the other reviewers results.

Except mentioned notes, I suppose the patch is good enough to pass it to a reviewer.
-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--------------090806020702080007000706--