Re: Optimizing ResouceOwner to speed up COPY

On 10/17/25 10:31, Heikki Linnakangas wrote:
> On 17/10/2025 06:13, Chao Li wrote:
>> ```
>> @@ -250,12 +257,21 @@ ResourceOwnerAddToHash(ResourceOwner owner,
>> Datum value, const ResourceOwnerDesc
>>       idx = hash_resource_elem(value, kind) & mask;
>>       for (;;)
>>       {
>> +        /* found an exact match - just increment the counter */
>> +        if ((owner->hash[idx].kind == kind) &&
>> +            (owner->hash[idx].item == value))
>> +        {
>> +            owner->hash[idx].count += count;
>> +            return;
>> +        }
>> +
>>           if (owner->hash[idx].kind == NULL)
>>               break;                /* found a free slot */
>>           idx = (idx + 1) & mask;
>>       }
>> ```
>>
>> I think this is the “trade-off” you mention in your email: if a free
>> slot found earlier, then still gets duplicated entries. I have no
>> concern to this “trade-off”, but please add a comment here, otherwise
>> future readers may be confused, and might potentially consider that
>> were a bug, without reading your original design email.
>
> +1 on such a comment. Here or maybe on the ResourceElem struct itself.
>

Will do. It's probably appropriate to mention this deduplication in the
comment at the beginning of the file.

>>  typedef struct ResourceElem
>>  {
>>      Datum        item;
>> +    uint32        count;                /* number of occurrences */
>>      const ResourceOwnerDesc *kind;    /* NULL indicates a free hash
>> table slot */
>>  } ResourceElem;
>
> Hmm, the 'count' is not used when the entry is stored in the array.
> Perhaps we should have a separate struct for array and hash elements
> now. Keeping the array small helps it to fit in CPU caches.
>

Agreed. I had the same idea yesterday, but I haven't done it yet.

>> /*
>>  * Forget that an object is owned by a ResourceOwner
>>  *
>>  * Note: If same resource ID is associated with the ResourceOwner more
>> than
>>  * once, one instance is removed.
>>  *
>>  * Note: Forgetting a resource does not guarantee that there is room to
>>  * remember a new resource.  One exception is when you forget the most
>>  * recently remembered resource; that does make room for a new
>> remember call.
>>  * Some code callers rely on that exception.
>>  */
>> void
>> ResourceOwnerForget(ResourceOwner owner, Datum value, const
>> ResourceOwnerDesc *kind)
>
> Does this break the exception noted in the above comment? I guess it
> still holds because we don't deduplicate entries in the array. That's
> very subtle, needs a comment at least.
>

Right, it doesn't break the exception - as you say, it only applies to
the hash, not to the array. I'll add a comment.

Thanks

--
Tomas Vondra