At Fri, 11 Mar 2022 15:30:30 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
> Thanks! I looked into dynahash part.
>
> struct HASHHDR
> {
> - /*
> - * The freelist can become a point of contention in high-concurrency hash
>
> Why did you move around the freeList?
>
>
> - long nentries; /* number of entries in associated buckets */
> + long nfree; /* number of free entries in the list */
> + long nalloced; /* number of entries initially allocated for
>
> Why do we need nfree? HASH_ASSING should do the same thing with
> HASH_REMOVE. Maybe the reason is the code tries to put the detached
> bucket to different free list, but we can just remember the
> freelist_idx for the detached bucket as we do for hashp. I think that
> should largely reduce the footprint of this patch.
>
> -static void hdefault(HTAB *hashp);
> +static void hdefault(HTAB *hashp, bool partitioned);
>
> That optimization may work even a bit, but it is not irrelevant to
> this patch?
>
> + case HASH_REUSE:
> + if (currBucket != NULL)
> + {
> + /* check there is no unfinished HASH_REUSE+HASH_ASSIGN pair */
> + Assert(DynaHashReuse.hashp == NULL);
> + Assert(DynaHashReuse.element == NULL);
>
> I think all cases in the switch(action) other than HASH_ASSIGN needs
> this assertion and no need for checking both, maybe only for element
> would be enough.
While I looked buf_table part, I came up with additional comments.
BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id)
{
hash_search_with_hash_value(SharedBufHash,
HASH_ASSIGN,
...
BufTableDelete(BufferTag *tagPtr, uint32 hashcode, bool reuse)
BufTableDelete considers both reuse and !reuse cases but
BufTableInsert doesn't and always does HASH_ASSIGN. That looks
odd. We should use HASH_ENTER here. Thus I think it is more
reasonable that HASH_ENTRY uses the stashed entry if exists and
needed, or returns it to freelist if exists but not needed.
What do you think about this?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
