On Wed, Mar 13, 2024 at 04:40:38PM +0300, Teodor Sigaev wrote: > Done, all patches should be applied consequentially.
One thing that first pops out to me is that we can do the refactor of hash_initial_lookup() as an independent piece, without the extra paths introduced. But rather than returning the bucket hash and have the bucket number as an in/out argument of hash_initial_lookup(), there is an argument for reversing them: hash_search_with_hash_value() does not care about the bucket number. > 02-hash_seq_init_with_hash_value.v5.patch - introduces a > hash_seq_init_with_hash_value() method. hash_initial_lookup() is marked as > inline, but I suppose, modern compilers are smart enough to inline it > automatically. Likely so, though that does not hurt to show the intention to the reader. So I would like to suggest the attached patch for this first piece. What do you think? It may also be an idea to use `git format-patch` when generating a series of patches. That makes for easier reviews. -- Michael
From 6b1fe126b9f72ff27aca08128948f4e617ba70dd Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Thu, 14 Mar 2024 16:35:40 +0900
Subject: [PATCH] Refactor initial hash lookup in dynahash.c
---
src/backend/utils/hash/dynahash.c | 75 ++++++++++++++-----------------
1 file changed, 33 insertions(+), 42 deletions(-)
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index a4152080b5..e1bd92a01c 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -273,6 +273,8 @@ static void hdefault(HTAB *hashp);
static int choose_nelem_alloc(Size entrysize);
static bool init_htab(HTAB *hashp, long nelem);
static void hash_corrupted(HTAB *hashp);
+static uint32 hash_initial_lookup(HTAB *hashp, uint32 hashvalue,
+ HASHBUCKET **bucketptr);
static long next_pow2_long(long num);
static int next_pow2_int(long num);
static void register_seq_scan(HTAB *hashp);
@@ -972,10 +974,6 @@ hash_search_with_hash_value(HTAB *hashp,
HASHHDR *hctl = hashp->hctl;
int freelist_idx = FREELIST_IDX(hctl, hashvalue);
Size keysize;
- uint32 bucket;
- long segment_num;
- long segment_ndx;
- HASHSEGMENT segp;
HASHBUCKET currBucket;
HASHBUCKET *prevBucketPtr;
HashCompareFunc match;
@@ -1008,17 +1006,7 @@ hash_search_with_hash_value(HTAB *hashp,
/*
* Do the initial lookup
*/
- bucket = calc_bucket(hctl, hashvalue);
-
- segment_num = bucket >> hashp->sshift;
- segment_ndx = MOD(bucket, hashp->ssize);
-
- segp = hashp->dir[segment_num];
-
- if (segp == NULL)
- hash_corrupted(hashp);
-
- prevBucketPtr = &segp[segment_ndx];
+ (void) hash_initial_lookup(hashp, hashvalue, &prevBucketPtr);
currBucket = *prevBucketPtr;
/*
@@ -1159,14 +1147,10 @@ hash_update_hash_key(HTAB *hashp,
const void *newKeyPtr)
{
HASHELEMENT *existingElement = ELEMENT_FROM_KEY(existingEntry);
- HASHHDR *hctl = hashp->hctl;
uint32 newhashvalue;
Size keysize;
uint32 bucket;
uint32 newbucket;
- long segment_num;
- long segment_ndx;
- HASHSEGMENT segp;
HASHBUCKET currBucket;
HASHBUCKET *prevBucketPtr;
HASHBUCKET *oldPrevPtr;
@@ -1187,17 +1171,8 @@ hash_update_hash_key(HTAB *hashp,
* this to be able to unlink it from its hash chain, but as a side benefit
* we can verify the validity of the passed existingEntry pointer.
*/
- bucket = calc_bucket(hctl, existingElement->hashvalue);
-
- segment_num = bucket >> hashp->sshift;
- segment_ndx = MOD(bucket, hashp->ssize);
-
- segp = hashp->dir[segment_num];
-
- if (segp == NULL)
- hash_corrupted(hashp);
-
- prevBucketPtr = &segp[segment_ndx];
+ bucket = hash_initial_lookup(hashp, existingElement->hashvalue,
+ &prevBucketPtr);
currBucket = *prevBucketPtr;
while (currBucket != NULL)
@@ -1219,18 +1194,7 @@ hash_update_hash_key(HTAB *hashp,
* chain we want to put the entry into.
*/
newhashvalue = hashp->hash(newKeyPtr, hashp->keysize);
-
- newbucket = calc_bucket(hctl, newhashvalue);
-
- segment_num = newbucket >> hashp->sshift;
- segment_ndx = MOD(newbucket, hashp->ssize);
-
- segp = hashp->dir[segment_num];
-
- if (segp == NULL)
- hash_corrupted(hashp);
-
- prevBucketPtr = &segp[segment_ndx];
+ newbucket = hash_initial_lookup(hashp, newhashvalue, &prevBucketPtr);
currBucket = *prevBucketPtr;
/*
@@ -1741,6 +1705,33 @@ element_alloc(HTAB *hashp, int nelem, int freelist_idx)
return true;
}
+/*
+ * Do initial lookup of a bucket for the given hash value, retrieving its
+ * bucket number and its hash bucket.
+ */
+static inline uint32
+hash_initial_lookup(HTAB *hashp, uint32 hashvalue, HASHBUCKET **bucketptr)
+{
+ HASHHDR *hctl = hashp->hctl;
+ HASHSEGMENT segp;
+ long segment_num;
+ long segment_ndx;
+ uint32 bucket;
+
+ bucket = calc_bucket(hctl, hashvalue);
+
+ segment_num = bucket >> hashp->sshift;
+ segment_ndx = MOD(bucket, hashp->ssize);
+
+ segp = hashp->dir[segment_num];
+
+ if (segp == NULL)
+ hash_corrupted(hashp);
+
+ *bucketptr = &segp[segment_ndx];
+ return bucket;
+}
+
/* complain when we have detected a corrupted hashtable */
static void
hash_corrupted(HTAB *hashp)
--
2.43.0
signature.asc
Description: PGP signature
