Re: ResourceOwner refactoring

At Thu, 19 Nov 2020 17:27:18 +0800, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote in
> On Thu, Nov 19, 2020 at 10:16 AM Michael Paquier <michael(at)paquier(dot)xyz> 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