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