ResourceOwner refactoring

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

ResourceOwner refactoring

Heikki Linnakangas
Two recent patches that I have reviewed have reminded me that the
ResourceOwner interface is a bit clunky. There are two issues:

1. Performance. It's fast enough in most use, but when I was testing
Kyotaro's catcache patches [1], the Resowner functions to track catcache
references nevertheless showed up, accounting for about 20% of the CPU
time used by catcache lookups.

2. It's difficult for extensions to use. There is a callback mechanism
for extensions, but it's much less convenient to use than the built-in
functions. The pgcrypto code uses the callbacks currently, and Michael's
patch [2] would move that support for tracking OpenSSL contexts to the
core, which makes it a lot more convenient for pgcrypto. Wouldn't it be
nice if extensions could have the same ergonomics as built-in code, if
they need to track external resources?

Attached patch refactors the ResourceOwner internals to do that.

The current code in HEAD has a separate array for each "kind" of object
that can be tracked. The number of different kinds of objects has grown
over the years, currently there is support for tracking buffers, files,
catcache, relcache and plancache references, tupledescs, snapshots, DSM
segments and LLVM JIT contexts. And locks, which are a bit special.

In the patch, I'm using the same array to hold all kinds of objects, and
carry another field with each tracked object, to tell what kind of an
object it is. An extension can define a new object kind, by defining
Release and PrintLeakWarning callback functions for it. The code in
resowner.c is now much more generic, as it doesn't need to know about
all the different object kinds anymore (with a couple of exceptions). In
the new scheme, there is one small fixed-size array to hold a few most
recently-added references, that is linear-searched, and older entries
are moved to a hash table.

I haven't done thorough performance testing of this, but with some quick
testing with Kyotaro's "catcachebench" to stress-test syscache lookups,
this performs a little better. In that test, all the activity happens in
the small array portion, but I believe that's true for most use cases.

Thoughts? Can anyone suggest test scenarios to verify the performance of
this?

[1]
https://www.postgresql.org/message-id/20201106.172958.1103727352904717607.horikyota.ntt%40gmail.com

[2] https://www.postgresql.org/message-id/20201113031429.GB1631@...

- Heikki

v1-resowner-refactor.patch (78K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Michael Paquier-2
On Tue, Nov 17, 2020 at 04:21:29PM +0200, Heikki Linnakangas wrote:
> 2. It's difficult for extensions to use. There is a callback mechanism for
> extensions, but it's much less convenient to use than the built-in
> functions. The pgcrypto code uses the callbacks currently, and Michael's
> patch [2] would move that support for tracking OpenSSL contexts to the core,
> which makes it a lot more convenient for pgcrypto. Wouldn't it be nice if
> extensions could have the same ergonomics as built-in code, if they need to
> track external resources?

+1.  True that the current interface is a bit hard to grasp, one can
easily miss that showing reference leaks is very important if an
allocation happens out of the in-core palloc machinery at commit time.
(The patch you are referring to is not really popular, but that does
not prevent this thread to move on on its own.)

> Attached patch refactors the ResourceOwner internals to do that.

+ * Size of the small fixed-size array to hold most-recently remembered resources.
  */
-#define RESARRAY_INIT_SIZE 16
+#define RESOWNER_ARRAY_SIZE 8
Why did you choose this size for the initial array?

+extern const char *ResourceOwnerGetName(ResourceOwner owner);
This is only used in resowner.c, at one place.

@@ -140,7 +139,6 @@ jit_release_context(JitContext *context)
    if (provider_successfully_loaded)
            provider.release_context(context);

-   ResourceOwnerForgetJIT(context->resowner, PointerGetDatum(context));
[...]
+   ResourceOwnerForget(context->resowner, PointerGetDatum(context), &jit_funcs);
This moves the JIT context release from jit.c to llvm.c.  I think
that's indeed more consistent, and correct.  It would be better to
check this one with Andres, though.

> I haven't done thorough performance testing of this, but with some quick
> testing with Kyotaro's "catcachebench" to stress-test syscache lookups, this
> performs a little better. In that test, all the activity happens in the
> small array portion, but I believe that's true for most use cases.



> Thoughts? Can anyone suggest test scenarios to verify the performance of
> this?

Playing with catcache.c is the first thing that came to my mind.
After that some micro-benchmarking with DSM or snapshots?  I am not
sure if we would notice a difference in a real-life scenario, say even
a pgbench -S with prepared queries.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Heikki Linnakangas
On 18/11/2020 10:06, Michael Paquier wrote:
> On Tue, Nov 17, 2020 at 04:21:29PM +0200, Heikki Linnakangas wrote:
>> Attached patch refactors the ResourceOwner internals to do that.
>
> + * Size of the small fixed-size array to hold most-recently remembered resources.
>    */
> -#define RESARRAY_INIT_SIZE 16
> +#define RESOWNER_ARRAY_SIZE 8
> Why did you choose this size for the initial array?

Just a guess. The old init size was 16 Datums. The entries in the new
array are twice as large, Datum+pointer.

The "RESOWNER_STATS" #ifdef blocks can be enabled to check how many
lookups fit in the array. With pgbench, RESOWNER_ARRAY_SIZE 8:

RESOWNER STATS: lookups: array 235, hash 32

If RESOWNER_ARRAY_STATS is increased to 16, all the lookups fit in the
array. But I haven't done any benchmarking to see which is faster.

BTW, I think there would be an easy win in the hashing codepath, by
changing to a cheaper hash function. Currently, with or without this
patch, we use hash_any(). Changing that to murmurhash32() or something
similar would be a drop-in replacement.

- Heikki


Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Michael Paquier-2
On Wed, Nov 18, 2020 at 10:50:08AM +0200, Heikki Linnakangas wrote:
> If RESOWNER_ARRAY_STATS is increased to 16, all the lookups fit in the
> array. But I haven't done any benchmarking to see which is faster.

My gut tells me that your guess is right, but it would be better to be
sure.

> BTW, I think there would be an easy win in the hashing codepath, by changing
> to a cheaper hash function. Currently, with or without this patch, we use
> hash_any(). Changing that to murmurhash32() or something similar would be a
> drop-in replacement.

Good idea.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Julien Rouhaud
On Thu, Nov 19, 2020 at 10:16 AM Michael Paquier <[hidden email]> wrote:

>
> On Wed, Nov 18, 2020 at 10:50:08AM +0200, Heikki Linnakangas wrote:
> > If RESOWNER_ARRAY_STATS is increased to 16, all the lookups fit in the
> > array. But I haven't done any benchmarking to see which is faster.
>
> My gut tells me that your guess is right, but it would be better to be
> sure.
>
> > BTW, I think there would be an easy win in the hashing codepath, by changing
> > to a cheaper hash function. Currently, with or without this patch, we use
> > hash_any(). Changing that to murmurhash32() or something similar would be a
> > drop-in replacement.
>
> Good idea.

+1, and +1 for this refactoring.

I just saw a minor issue in a comment while reviewing the patch:

[...]
+ /* Is there space in the hash? If not, enlarge it. */

  /* Double the capacity of the array (capacity must stay a power of 2!) */
- newcap = (oldcap > 0) ? oldcap * 2 : RESARRAY_INIT_SIZE;
[...]

The existing comment is kept as-is, but it should mention that it's
now the hash capacity that is increased.


Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Craig Ringer-5
In reply to this post by Heikki Linnakangas
[off-list for now]

On Tue, Nov 17, 2020 at 10:21 PM Heikki Linnakangas <[hidden email]> wrote:

2. It's difficult for extensions to use. There is a callback mechanism
for extensions, but it's much less convenient to use than the built-in
functions. The pgcrypto code uses the callbacks currently, and Michael's
patch [2] would move that support for tracking OpenSSL contexts to the
core, which makes it a lot more convenient for pgcrypto. Wouldn't it be
nice if extensions could have the same ergonomics as built-in code, if
they need to track external resources?

Yes, in general I'm a big fan of making life easier for extensions (see postscript), and this looks helpful. It's certainly fairly clear to read. I'm in-principle in favour, though I haven't read the old resowner code closely enough to have a strong opinion.

I haven't done thorough performance testing of this, but with some quick
testing with Kyotaro's "catcachebench" to stress-test syscache lookups,
this performs a little better. In that test, all the activity happens in
the small array portion, but I believe that's true for most use cases.

Thoughts? Can anyone suggest test scenarios to verify the performance of
this?

I didn't think of much really.

I have tossed together a patch on top of yours that adds some systemtap/dtrace tracepoints and provides a demo systemtap script that shows some basic stats collection done using them. My intention was to make it easier to see where the work in the resource owner code was being done. But after I did the initial version I realised that in this case it probably doesn't add a lot of value over using targeted profiling of only functions in resowner.c and their callees which is easy enough using perf and regular DWARF debuginfo. So I didn't land up polishing it and adding stats for the other counters. It's attached anyway, in case it's interesting or useful to anyone.

P.S. At some point I want to tackle some things similar to what you've done here for other kinds of backend resources. In particular, to allow extensions to register their own heavyweight lock types - with the ability to handle lock timeouts, and add callbacks in the deadlock detector to allow extensions to register dependency edges that are not natively visible to the deadlock detector and to choose victim priorities. Also some enhancements to how pg_depend tracking can be used by extensions. And I want to help get the proposed custom ProcSignal changes in too. I think there's a whole lot to be done to make extensions easier and safer to author in general, like providing a simple way to do error trapping and recovery in an extension mainloop without copy/pasting a bunch of PostgresMain code, better default signal handlers, startup/shutdown that shares more with user backends, etc. Right now it's quite tricky to get bgworkers to behave well. </wildhandwaving>

0001-Poc-of-systemtap-probes-for-resowner-timings.patch (19K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Michael Paquier-2
On Thu, Nov 19, 2020 at 06:40:19PM +0800, Craig Ringer wrote:
> [off-list for now]

Looks like you have missed something here.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Kyotaro Horiguchi-4
In reply to this post by Julien Rouhaud
At Thu, 19 Nov 2020 17:27:18 +0800, Julien Rouhaud <[hidden email]> wrote in

> On Thu, Nov 19, 2020 at 10:16 AM Michael Paquier <[hidden email]> wrote:
> >
> > On Wed, Nov 18, 2020 at 10:50:08AM +0200, Heikki Linnakangas wrote:
> > > If RESOWNER_ARRAY_STATS is increased to 16, all the lookups fit in the
> > > array. But I haven't done any benchmarking to see which is faster.
> >
> > My gut tells me that your guess is right, but it would be better to be
> > sure.
> >
> > > BTW, I think there would be an easy win in the hashing codepath, by changing
> > > to a cheaper hash function. Currently, with or without this patch, we use
> > > hash_any(). Changing that to murmurhash32() or something similar would be a
> > > drop-in replacement.
> >
> > Good idea.
>
> +1, and +1 for this refactoring.

+1 for making the interface more generic. I thought a similar thing
when I added an resowner array for WaitEventSet (not committed yet).
About performance, though I'm not sure about, there's no reason not to
do this as far as the resowner mechanism doesn't get slower, and +2 if
gets faster.

> I just saw a minor issue in a comment while reviewing the patch:
>
> [...]
> + /* Is there space in the hash? If not, enlarge it. */
>
>   /* Double the capacity of the array (capacity must stay a power of 2!) */
> - newcap = (oldcap > 0) ? oldcap * 2 : RESARRAY_INIT_SIZE;
> [...]
>
> The existing comment is kept as-is, but it should mention that it's
> now the hash capacity that is increased.

+ /* And release old array. */
+ pfree(oldhash);

:p


+ for (int i = 0; i < owner->narr; i++)
  {
+ if (owner->arr[i].kind->phase == phase)
+ {
+ Datum value = owner->arr[i].item;
+ ResourceOwnerFuncs *kind = owner->arr[i].kind;
+
+ if (printLeakWarnings)
+ kind->PrintLeakWarning(value);
+ kind->ReleaseResource(value);
+ found = true;
+ }
+ /*
+ * If any resources were released, check again because some of the
+ * elements might have moved by the callbacks. We don't want to
+ * miss them.
+ */
+ } while (found && owner->narr > 0);

Coundn't that missing be avoided by just not incrementing i if found?


+ /*
+ * Like with the array, we must check again after we reach the
+ * end, if any callbacks were called. XXX: We could probably
+ * stipulate that the callbacks may not do certain thing, like
+ * remember more references in the same resource owner, to avoid
+ * that.
+ */

If I read this patch correctly, ResourceOwnerForget doesn't seem to do
such a thing for hash?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Heikki Linnakangas
On 26/11/2020 10:52, Kyotaro Horiguchi wrote:

> + for (int i = 0; i < owner->narr; i++)
>   {
> + if (owner->arr[i].kind->phase == phase)
> + {
> + Datum value = owner->arr[i].item;
> + ResourceOwnerFuncs *kind = owner->arr[i].kind;
> +
> + if (printLeakWarnings)
> + kind->PrintLeakWarning(value);
> + kind->ReleaseResource(value);
> + found = true;
> + }
> + /*
> + * If any resources were released, check again because some of the
> + * elements might have moved by the callbacks. We don't want to
> + * miss them.
> + */
> + } while (found && owner->narr > 0);
>
> Coundn't that missing be avoided by just not incrementing i if found?
Hmm, perhaps. ResourceOwnerForget() can move an entry from the end of
the array to the beginning, but if it's removing the entry that we're
just looking at, it probably can't move anything before that entry. I'm
not very comfortable with that, though. What if the callback releases
something else as a side effect?

This isn't super-critical for performance, and given that the array is
very small, it's very cheap to loop through it. So I'm inclined to keep
it safe.

> + /*
> + * Like with the array, we must check again after we reach the
> + * end, if any callbacks were called. XXX: We could probably
> + * stipulate that the callbacks may not do certain thing, like
> + * remember more references in the same resource owner, to avoid
> + * that.
> + */
>
> If I read this patch correctly, ResourceOwnerForget doesn't seem to do
> such a thing for hash?
Hmm, true. I tried removing the loop and hit another issue, however: if
the same resource has been remembered twice in the same resource owner,
the callback might remove different reference to it than what we're
looking at. So we need to loop anyway to handle that. Also, what if the
callback remembers some other resource in the same resowner, causing the
hash to grow? I'm not sure if any of the callbacks currently do that,
but better safe than sorry. I updated the code and the comments accordingly.


Here's an updated version of this patch. I fixed the bit rot, and
addressed all the other comment issues and such that people pointed out
(thanks!).

TODO:

1. Performance testing. We discussed this a little bit, but I haven't
done any more testing.

2. Before this patch, the ResourceOwnerEnlarge() calls enlarged the
array for the particular "kind" of resource, but now they are all stored
in the same structure. That could lead to trouble if we do something
like this:

ResourceOwnerEnlargeAAA()
ResourceOwnerEnlargeBBB()
ResourceOwnerRememberAAA()
ResourceOwnerRememberBBB()

Previously, this was OK, because resources AAA and BBB were kept in
separate arrays. But after this patch, it's not guaranteed that the
ResourceOwnerRememberBBB() will find an empty slot.

I don't think we do that currently, but to be sure, I'm planning to grep
ResourceOwnerRemember and look at each call carefullly. And perhaps we
can add an assertion for this, although I'm not sure where.

- Heikki

v2-0001-Make-resowners-more-easily-extensible.patch (84K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: ResourceOwner refactoring

kuroda.hayato@fujitsu.com
Dear Heikki,

I'm also interested in this patch, but it cannot be applied to the current HEAD...

$ git apply ~/v2-0001-Make-resowners-more-easily-extensible.patch
error: patch failed: src/common/cryptohash_openssl.c:57
error: src/common/cryptohash_openssl.c: patch does not apply
error: patch failed: src/include/utils/resowner_private.h:1
error: src/include/utils/resowner_private.h: patch does not apply

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Heikki Linnakangas
On 13/01/2021 03:55, [hidden email] wrote:
> Dear Heikki,
>
> I'm also interested in this patch, but it cannot be applied to the current HEAD...
>
> $ git apply ~/v2-0001-Make-resowners-more-easily-extensible.patch
> error: patch failed: src/common/cryptohash_openssl.c:57
> error: src/common/cryptohash_openssl.c: patch does not apply
> error: patch failed: src/include/utils/resowner_private.h:1
> error: src/include/utils/resowner_private.h: patch does not apply

Here's a rebased version. Thanks!

- Heikki

v3-0001-Make-resowners-more-easily-extensible.patch (84K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Michael Paquier-2
On Wed, Jan 13, 2021 at 09:18:57AM +0200, Heikki Linnakangas wrote:

> --- a/src/common/cryptohash_openssl.c
> +++ b/src/common/cryptohash_openssl.c
> +static ResourceOwnerFuncs cryptohash_funcs =
> +{
> + /* relcache references */
> + .name = "LLVM JIT context",
> + .phase = RESOURCE_RELEASE_BEFORE_LOCKS,
> + .ReleaseResource = ResOwnerReleaseCryptoHash,
> + .PrintLeakWarning = ResOwnerPrintCryptoHashLeakWarning,
> +};
> +#endif
Looks like a copy-paste error here.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: ResourceOwner refactoring

kuroda.hayato@fujitsu.com
In reply to this post by Heikki Linnakangas
Dear Heikki,

Thank you for rebasing it, I confirmed it can be applied.
I will check the source.

Now I put the very elementary comment.
ResourceOwnerEnlarge(), ResourceOwnerRemember(), and ResourceOwnerForget()
are exported routines.
They should put below L418.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Reply | Threaded
Open this post in threaded view
|

RE: ResourceOwner refactoring

kuroda.hayato@fujitsu.com
Hi,

I put some comments.

Throughout, some components don’t have helper functions.
(e.g. catcache has ResOwnerReleaseCatCache, but tupdesc doesn't.)
I think it should be unified.

[catcache.c]
> +/* support for catcache refcount management */
> +static inline void
> +ResourceOwnerEnlargeCatCacheRefs(ResourceOwner owner)
> +{
> +       ResourceOwnerEnlarge(owner);
> +}

This function is not needed.

[syscache.c]
> -static CatCache *SysCache[SysCacheSize];
> + CatCache *SysCache[SysCacheSize];

Is it right? Compilation is done even if this variable is static...

[fd.c, dsm.c]
In these files helper functions are implemented as the define directive.
Could you explain the reason? For the performance?

> Previously, this was OK, because resources AAA and BBB were kept in
> separate arrays. But after this patch, it's not guaranteed that the
> ResourceOwnerRememberBBB() will find an empty slot.
>
> I don't think we do that currently, but to be sure, I'm planning to grep
> ResourceOwnerRemember and look at each call carefullly. And perhaps we
> can add an assertion for this, although I'm not sure where.

Indeed, but I think this line works well, isn't it?

> Assert(owner->narr < RESOWNER_ARRAY_SIZE);

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Heikki Linnakangas
On 14/01/2021 12:15, [hidden email] wrote:
> I put some comments.

Thanks for the review!

> Throughout, some components don’t have helper functions.
> (e.g. catcache has ResOwnerReleaseCatCache, but tupdesc doesn't.)
> I think it should be unified.

Hmm. ResOwnerReleaseTupleDesc() does exist, those functions are needed
for the callbacks. I think you meant the wrappers around
ResourceOwnerRemember and ResourceOwnerForget, like
ResourceOwnerRememberCatCacheRef(). I admit those are not fully
consistent: I didn't bother with the wrapper functions when there is
only one caller.

> [catcache.c]
>> +/* support for catcache refcount management */
>> +static inline void
>> +ResourceOwnerEnlargeCatCacheRefs(ResourceOwner owner)
>> +{
>> +       ResourceOwnerEnlarge(owner);
>> +}
>
> This function is not needed.

Removed.

> [syscache.c]
>> -static CatCache *SysCache[SysCacheSize];
>> + CatCache *SysCache[SysCacheSize];
>
> Is it right? Compilation is done even if this variable is static...

Fixed. (It was a leftover from when I was playing with Kyotaro's
"catcachebench" tool from another thread).

> [fd.c, dsm.c]
> In these files helper functions are implemented as the define directive.
> Could you explain the reason? For the performance?

No particular reason. I turned them all into macros for consistency.

>> Previously, this was OK, because resources AAA and BBB were kept in
>> separate arrays. But after this patch, it's not guaranteed that the
>> ResourceOwnerRememberBBB() will find an empty slot.
>>
>> I don't think we do that currently, but to be sure, I'm planning to grep
>> ResourceOwnerRemember and look at each call carefullly. And perhaps we
>> can add an assertion for this, although I'm not sure where.
>
> Indeed, but I think this line works well, isn't it?
>
>> Assert(owner->narr < RESOWNER_ARRAY_SIZE);
That catches cases where you actually overrun the array, but it doesn't
catch unsafe patterns when there happens to be enough space left in the
array. For example, if you have code like this:

/* Make sure there's room for one more entry, but remember *two* things */
ResourceOwnerEnlarge();
ResourceOwnerRemember(foo);
ResourceOwnerRemember(bar);

That is not safe, but it would only fail the assertion if the first
ResourceOwnerRemember() call happens to consume the last remaining slot
in the array.


I checked all the callers of ResourceOwnerEnlarge() to see if they're
safe. A couple of places seemed a bit suspicious. I fixed them by moving
the ResourceOwnerEnlarge() calls closer to the ResourceOwnerRemember()
calls, so that it's now easier to see that they are correct. See first
attached patch.

The second patch is an updated version of the main patch, fixing all the
little things you and Michael pointed out since the last patch version.

I've been working on performance testing too. I'll post more numbers
later, but preliminary result from some micro-benchmarking suggests that
the new code is somewhat slower, except in the common case that the
object to remember and forget fits in the array. When running the
regression test suite, about 96% of ResourceOwnerForget() calls fit in
the array. I think that's acceptable.

- Heikki

v4-0001-Move-a-few-ResourceOwnerEnlarge-calls-for-safety-.patch (8K) Download Attachment
v4-0002-Make-resowners-more-easily-extensible.patch (86K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: ResourceOwner refactoring

kuroda.hayato@fujitsu.com
Dear Heikki,

> Hmm. ResOwnerReleaseTupleDesc() does exist, those functions are needed
> for the callbacks. I think you meant the wrappers around
> ResourceOwnerRemember and ResourceOwnerForget, like
> ResourceOwnerRememberCatCacheRef(). I admit those are not fully
> consistent: I didn't bother with the wrapper functions when there is
> only one caller.

Yeah, I meant it. And I prefer your policy.

> Hmm. ResOwnerReleaseTupleDesc() does exist, those functions are needed
> for the callbacks. I think you meant the wrappers around
> ResourceOwnerRemember and ResourceOwnerForget, like
> ResourceOwnerRememberCatCacheRef(). I admit those are not fully
> consistent: I didn't bother with the wrapper functions when there is
> only one caller.

Good job. I confirmed your fixes, and I confirmed it looks fine.
I will check another ResourceOwnerEnlarge() if I have a time.

> I've been working on performance testing too.

I'm looking forward to seeing it.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Reply | Threaded
Open this post in threaded view
|

RE: ResourceOwner refactoring

kuroda.hayato@fujitsu.com
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.

Hayato Kuroda
FUJITSU LIMITED

Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Heikki Linnakangas
On 18/01/2021 09:49, [hidden email] 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

v5-0001-Move-a-few-ResourceOwnerEnlarge-calls-for-safety-.patch (8K) Download Attachment
v5-0002-Make-resowners-more-easily-extensible.patch (87K) Download Attachment
0001-resownerbench.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Álvaro Herrera
On 2021-Jan-18, Heikki Linnakangas wrote:

> +static ResourceOwnerFuncs jit_funcs =
> +{
> + /* relcache references */
> + .name = "LLVM JIT context",
> + .phase = RESOURCE_RELEASE_BEFORE_LOCKS,
> + .ReleaseResource = ResOwnerReleaseJitContext,
> + .PrintLeakWarning = ResOwnerPrintJitContextLeakWarning
> +};

I think you mean jit_resowner_funcs here; "jit_funcs" is a bit
excessively vague.  Also, why did you choose not to define
ResourceOwnerRememberJIT?  You do that in other modules and it seems
better.

> +static ResourceOwnerFuncs catcache_funcs =
> +{
> + /* catcache references */
> + .name = "catcache reference",
> + .phase = RESOURCE_RELEASE_AFTER_LOCKS,
> + .ReleaseResource = ResOwnerReleaseCatCache,
> + .PrintLeakWarning = ResOwnerPrintCatCacheLeakWarning
> +};
> +
> +static ResourceOwnerFuncs catlistref_funcs =
> +{
> + /* catcache-list pins */
> + .name = "catcache list reference",
> + .phase = RESOURCE_RELEASE_AFTER_LOCKS,
> + .ReleaseResource = ResOwnerReleaseCatCacheList,
> + .PrintLeakWarning = ResOwnerPrintCatCacheListLeakWarning
> +};

Similar naming issue here, I say.


> diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
> index 551ec392b60..642e72d8c0f 100644
> --- a/src/common/cryptohash_openssl.c
> +++ b/src/common/cryptohash_openssl.c
> @@ -60,6 +59,21 @@ struct pg_cryptohash_ctx
>  #endif
>  };
>  
> +/* ResourceOwner callbacks to hold JitContexts  */
> +#ifndef FRONTEND
> +static void ResOwnerReleaseCryptoHash(Datum res);
> +static void ResOwnerPrintCryptoHashLeakWarning(Datum res);

The comment is wrong -- "Crypohashes", not "JitContexts".


So according to your performance benchmark, we're willing to accept a
30% performance loss on an allegedly common operation -- numkeep=0
numsnaps=10 becomes 49.8ns from 37.6ns.  That seems a bit shocking.
Maybe you can claim that these operations aren't exactly hot spots, and
so the fact that we remain in the same power-of-ten is sufficient.  Is
that the argument?

--
Álvaro Herrera                            39°49'30"S 73°17'W
"The Postgresql hackers have what I call a "NASA space shot" mentality.
 Quite refreshing in a world of "weekend drag racer" developers."
(Scott Marlowe)


Reply | Threaded
Open this post in threaded view
|

Re: ResourceOwner refactoring

Heikki Linnakangas
On 18/01/2021 16:34, Alvaro Herrera wrote:
> So according to your performance benchmark, we're willing to accept a
> 30% performance loss on an allegedly common operation -- numkeep=0
> numsnaps=10 becomes 49.8ns from 37.6ns.  That seems a bit shocking.
> Maybe you can claim that these operations aren't exactly hot spots, and
> so the fact that we remain in the same power-of-ten is sufficient.  Is
> that the argument?

That's right. The fast path is fast, and that's important. The slow path
becomes 30% slower, but that's acceptable.

- Heikki


12