Re: ResourceOwner refactoring

On 18/01/2021 09:49, kuroda(dot)hayato(at)fujitsu(dot)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