Re: Division in dynahash.c due to HASH_FFACTOR - Mailing list pgsql-hackers

On Thu, 10 Sep 2020 at 14:55, Thomas Munro <thomas.munro@gmail.com> wrote:
>
> I wrote a draft commit message for Jakub's proposed change (attached),
> and did a little bit of testing, but I haven't seen a case where it
> wins yet; I need to work on something else now but thought I'd share
> this much anyway.  One observation is that the rule in init_htab had
> better agree with the expansion rule in hash_search_with_hash_value,
> otherwise you can size your hash table perfectly for n elements and
> then it still has to expand when you insert the nth element, which is
> why I changed >= to >.  Does this make sense?  Oh man, I don't like
> the mash-up of int, long, uint32, Size dynahash uses in various places
> and that are brought into relief by that comparison...

I just did some benchmarking with this patch using the same recovery
benchmark that I used in [1] and also the two patches that I posted in
[2]. Additionally, I added a PANIC at the end of recovery so that I
could repeat the recovery over and over again with the same WAL.

Without 0001-Remove-custom-fill-factor-support-from-dynahash.c.patch,
the time in seconds reported for recovery was as follows:

Run 1 65.2
Run 2 65.41
Run 3 65.1
Run 4 64.86
Run 5 62.29
Run 6 67.06
Run 7 63.35
Run 8 63.1
Run 9 62.8
Run 10 62.15

Avg 64.132
Med 64.105

After patching I got:

Run 1 60.69
Run 2 63.13
Run 3 63.24
Run 4 62.13
Run 5 63.81
Run 6 60.27
Run 7 62.85
Run 8 63.45
Run 9 59.6
Run 10 63.16

Avg 62.233
Med 62.99

I was quite happy to see 59.6 seconds in there on run 9 as I'd been
trying hard to get that benchmark to go below 1 minute on my machine.

perf appears to indicate that less time was spent in
hash_search_with_hash_value()

Unpatched:
  26.96%  postgres  postgres            [.] PageRepairFragmentation
  12.07%  postgres  libc-2.31.so        [.] __memmove_avx_unaligned_erms
  10.13%  postgres  postgres            [.] hash_search_with_hash_value
   6.70%  postgres  postgres            [.] XLogReadBufferExtended

Patched:
  27.70%  postgres  postgres            [.] PageRepairFragmentation
  13.50%  postgres  libc-2.31.so        [.] __memmove_avx_unaligned_erms
   9.63%  postgres  postgres            [.] hash_search_with_hash_value
   6.44%  postgres  postgres            [.] XLogReadBufferExtended

I looked over the patch and the only thing I saw was that we might
also want to remove the following line:

#define DEF_FFACTOR    1 /* default fill factor */

Also, a couple of things I noticed when looking at performance
profiles in detail.

1) The most expensive user of hash_search_with_hash_value() comes from
BufTableLookup() which passes HASH_FIND as the HASHACTION.
2) The code that was doing the divide in the unpatched version only
triggered when HASHACTION was HASH_ENTER or HASH_ENTER_NULL.

The 2nd most costly call to hash_search_with_hash_value() came in via
hash_search() via smgropen(). That does use HASH_ENTER, which could
have triggered the divide code. The main caller of smgropen() was
XLogReadBufferExtended().

So, it looks unlikely that any gains we are seeing are from improved
buffer lookups. It's more likely they're coming from more optimal
XLogReadBufferExtended()

David

[1] https://www.postgresql.org/message-id/CAApHDvoKwqAzhiuxEt8jSquPJKDpH8DNUZDFUSX9P7DXrJdc3Q@mail.gmail.com
[2] https://www.postgresql.org/message-id/CAApHDvo9n-nOb3b4PYFx%2Bw9mqd2SSUHm_oAs039eZnZLqFGcbQ%40mail.gmail.com