Re: ResourceOwner refactoring

On 18/01/2021 09:49, kuroda.hayato@fujitsu.com wrote:
> Dear Heikki,
> 
> I apologize for sending again.
> 
>> I will check another ResourceOwnerEnlarge() if I have a time.
> 
> I checked all ResourceOwnerEnlarge() types after applying patch 0001.
> (searched by "grep -rI ResourceOwnerEnlarge")
> No problem was found.

Great, thanks!

Here are more details on the performance testing I did:

Unpatched
----------

postgres=# \i snaptest.sql
  numkeep | numsnaps | lifo_time_ns | fifo_time_ns
---------+----------+--------------+--------------
        0 |        1 |         38.2 |         34.8
        0 |        5 |         35.7 |         35.4
        0 |       10 |         37.6 |         37.6
        0 |       60 |         35.4 |         39.2
        0 |       70 |         55.0 |         53.8
        0 |      100 |         48.6 |         48.9
        0 |     1000 |         54.8 |         57.0
        0 |    10000 |         63.9 |         67.1
        9 |       10 |         39.3 |         38.8
        9 |      100 |         44.0 |         42.5
        9 |     1000 |         45.8 |         47.1
        9 |    10000 |         64.2 |         66.8
       65 |       70 |         45.7 |         43.7
       65 |      100 |         42.9 |         43.7
       65 |     1000 |         46.9 |         45.7
       65 |    10000 |         65.0 |         64.5
(16 rows)


Patched
--------

postgres=# \i snaptest.sql
  numkeep | numsnaps | lifo_time_ns | fifo_time_ns
---------+----------+--------------+--------------
        0 |        1 |         35.3 |         33.8
        0 |        5 |         34.8 |         34.6
        0 |       10 |         49.8 |         51.4
        0 |       60 |         53.1 |         57.2
        0 |       70 |         53.2 |         59.6
        0 |      100 |         53.1 |         58.2
        0 |     1000 |         63.1 |         69.7
        0 |    10000 |         83.3 |         83.5
        9 |       10 |         35.0 |         35.1
        9 |      100 |         55.4 |         54.1
        9 |     1000 |         56.8 |         60.3
        9 |    10000 |         88.6 |         82.0
       65 |       70 |         36.4 |         35.1
       65 |      100 |         52.4 |         53.0
       65 |     1000 |         55.8 |         59.4
       65 |    10000 |         79.3 |         80.3
(16 rows)

About the test:

Each test call Register/UnregisterSnapshot in a loop. numsnaps is the 
number of snapshots that are registered in each iteration. For exmaple, 
on the second line with numkeep=0 and numnaps=5, each iteration in the 
LIFO test does essentially:

rs[0] = RegisterSnapshot()
rs[1] = RegisterSnapshot()
rs[2] = RegisterSnapshot()
rs[3] = RegisterSnapshot()
rs[4] = RegisterSnapshot()
UnregisterSnapshot(rs[4]);
UnregisterSnapshot(rs[3]);
UnregisterSnapshot(rs[2]);
UnregisterSnapshot(rs[1]);
UnregisterSnapshot(rs[0]);

In the FIFO tests, the Unregister calls are made in reverse order.

When numkeep is non zero, that many snapshots are registered once at the 
beginning of the test, and released only after the test loop. So this 
simulates the scenario that you remember a bunch of resources and hold 
them for a long time, and while holding them, you do many more 
remember/forget calls.

Based on this test, this patch makes things slightly slower overall. 
However, *not* in the cases that matter the most I believe. That's the 
cases where the (numsnaps - numkeep) is small, so that the hot action 
happens in the array and the hashing is not required. In my testing 
earlier, about 95% of the ResourceOwnerRemember calls in the regression 
tests fall into that category.

There are a few simple things we could do speed this up, if needed. A 
different hash function might be cheaper, for example. And we could 
inline the fast paths of the ResourceOwnerEnlarge and 
ResourceOwnerRemember() into the callers. But I didn't do those things 
as part of this patch, because it wouldn't be a fair comparison; we 
could do those with the old implementation too. And unless we really 
need the speed, it's more readable this way.

I'm OK with these results. Any objections?

Attached are the patches again. Same as previous version, except for a 
couple of little comment changes. And a third patch containing the 
source for the performance test.

- Heikki