Re: ResourceOwner refactoring

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