Hash table cleanup [3/n]

A few cleanups:
1. Move implementation details (macro definition, SHashEntry and SHashObj
   definition) to hash.c. Only leave relevant APIs in hash.h.
2. Complete function header comments.
3. Correct variable naming from "newSize" to "newCapacity".

src/query/src/qExecutor.c

@@ -9227,6 +9227,9 @@ static void doSetTagValueToResultBuf(char* output, const char* val, int16_t type
 
 static int64_t getQuerySupportBufSize(size_t numOfTables) {
   size_t s1 = sizeof(STableQueryInfo);
+
+  // TODO: struct SHashNode is an internal implementation of
+  // hash table. The implementation should not leak here.
   size_t s2 = sizeof(SHashNode);
 
 //  size_t s3 = sizeof(STableCheckInfo);  buffer consumption in tsdb

src/util/inc/hash.h

@@ -24,12 +24,8 @@ extern "C" {
 #include "hashfunc.h"
 #include "tlockfree.h"
 
-#define HASH_MAX_CAPACITY (1024 * 1024 * 16)
-#define HASH_DEFAULT_LOAD_FACTOR (0.75)
-#define HASH_INDEX(v, c) ((v) & ((c)-1))
-
-typedef void (*_hash_free_fn_t)(void *param);
-
+// TODO: SHashNode is an internal implementation and should not
+// be in the public header file.
 typedef struct SHashNode {
   struct SHashNode *next;
   uint32_t          hashVal;     // the hash value of key
@@ -40,48 +36,27 @@ typedef struct SHashNode {
   char              data[];
 } SHashNode;
 
-#define GET_HASH_NODE_KEY(_n)  ((char*)(_n) + sizeof(SHashNode) + (_n)->dataLen)
-#define GET_HASH_NODE_DATA(_n) ((char*)(_n) + sizeof(SHashNode))
-#define GET_HASH_PNODE(_n) ((SHashNode *)((char*)(_n) - sizeof(SHashNode)))
-
 typedef enum SHashLockTypeE {
   HASH_NO_LOCK     = 0,
   HASH_ENTRY_LOCK  = 1,
 } SHashLockTypeE;
 
-typedef struct SHashEntry {
-  int32_t    num;      // number of elements in current entry
-  SRWLatch   latch;    // entry latch
-  SHashNode *next;
-} SHashEntry;
-
-typedef struct SHashObj {
-  SHashEntry    **hashList;
-  size_t          capacity;     // number of slots
-  size_t          size;         // number of elements in hash table
-  _hash_fn_t      hashFp;       // hash function
-  _hash_free_fn_t freeFp;       // hash node free callback function
-  _equal_fn_t     equalFp;       // equal function
-
-  SRWLatch        lock;         // read-write spin lock
-  SHashLockTypeE  type;         // lock type
-  bool            enableUpdate; // enable update
-  SArray         *pMemBlock;    // memory block allocated for SHashEntry
-} SHashObj;
+typedef struct SHashObj SHashObj;
 
 /**
- * init the hash table
+ * initialize a hash table
  *
  * @param capacity   initial capacity of the hash table
- * @param fn          hash function to generate the hash value
- * @param threadsafe  thread safe or not
- * @return
+ * @param fn         hash function
+ * @param update     whether the hash table allows in place update
+ * @param type       whether the hash table has per entry lock
+ * @return           hash table object
  */
 SHashObj *taosHashInit(size_t capacity, _hash_fn_t fn, bool update, SHashLockTypeE type);
 
-
 /**
  * set equal func of the hash table
+ *
  * @param pHashObj
  * @param equalFp
  * @return
@@ -90,6 +65,7 @@ void taosHashSetEqualFp(SHashObj *pHashObj, _equal_fn_t fp);
 
 /**
  * return the size of hash table
+ *
  * @param pHashObj
  * @return
  */
@@ -97,73 +73,105 @@ int32_t taosHashGetSize(const SHashObj *pHashObj);
 
 /**
  * put element into hash table, if the element with the same key exists, update it
- * @param pHashObj
- * @param key
- * @param keyLen
- * @param data
- * @param size
- * @return
+ *
+ * @param pHashObj   hash table object
+ * @param key        key
+ * @param keyLen     length of key
+ * @param data       data
+ * @param size       size of data
+ * @return           0 if success, -1 otherwise
  */
 int32_t taosHashPut(SHashObj *pHashObj, const void *key, size_t keyLen, void *data, size_t size);
 
 /**
  * return the payload data with the specified key
  *
- * @param pHashObj
- * @param key
- * @param keyLen
- * @return
+ * @param pHashObj   hash table object
+ * @param key        key
+ * @param keyLen     length of key
+ * @return           pointer to data
  */
 void *taosHashGet(SHashObj *pHashObj, const void *key, size_t keyLen);
 
 /**
- * apply the udf before return the result
- * @param pHashObj
- * @param key
- * @param keyLen
- * @param fp
- * @param d
- * @return
+ * Get the data associated with "key". Note that caller needs to make sure
+ * "d" has enough capacity to accomodate the data.
+ *
+ * @param pHashObj   hash table object
+ * @param key        key
+ * @param keyLen     length of key
+ * @param fp         function to be called on hash node when the data is found
+ * @param d          buffer
+ * @return           pointer to data
  */
 void* taosHashGetClone(SHashObj *pHashObj, const void *key, size_t keyLen, void (*fp)(void *), void* d);
 
 /**
- * @param pHashObj
- * @param key
- * @param keyLen
- * @param fp
- * @param d
- * @param sz
- * @return
+ * Get the data associated with "key". Note that caller needs to take ownership
+ * of the data "d" and make sure it is deallocated.
+ *
+ * @param pHashObj   hash table object
+ * @param key        key
+ * @param keyLen     length of key
+ * @param fp         function to be called on hash node when the data is found
+ * @param d          buffer
+ * @param sz         size of the data buffer
+ * @return           pointer to data
  */
 void* taosHashGetCloneExt(SHashObj *pHashObj, const void *key, size_t keyLen, void (*fp)(void *), void** d, size_t *sz);
+
 /**
  * remove item with the specified key
- * @param pHashObj
- * @param key
- * @param keyLen
+ *
+ * @param pHashObj   hash table object
+ * @param key        key
+ * @param keyLen     length of key
+ * @return           0 if success, -1 otherwise
  */
 int32_t taosHashRemove(SHashObj *pHashObj, const void *key, size_t keyLen);
 
+/**
+ * remove item with the specified key
+ *
+ * @param pHashObj   hash table object
+ * @param key        key
+ * @param keyLen     length of key
+ * @param data       buffer for data
+ * @param dsize      size of data buffer
+ * @return           0 if success, -1 otherwise
+ */
 int32_t taosHashRemoveWithData(SHashObj *pHashObj, const void *key, size_t keyLen, void* data, size_t dsize);
 
 int32_t taosHashCondTraverse(SHashObj *pHashObj, bool (*fp)(void *, void *), void *param);
 
+/**
+ * clear the contents of the hash table
+ *
+ * @param pHashObj   hash table object
+ */
 void taosHashClear(SHashObj *pHashObj);
 
 /**
  * clean up hash table
- * @param handle
+ *
+ * @param pHashObj   hash table object
  */
 void taosHashCleanup(SHashObj *pHashObj);
 
 /**
+ * return the number of collisions in the hash table
  *
- * @param pHashObj
- * @return
+ * @param pHashObj   hash table object
+ * @return           maximum number of collisions
  */
 int32_t taosHashGetMaxOverflowLinkLength(const SHashObj *pHashObj);
 
+/**
+ * return the consumed memory of the hash table
+ *
+ * @param pHashObj   hash table object
+ * @return           consumed memory of the hash table
+ */
 size_t taosHashGetMemSize(const SHashObj *pHashObj);
 
 void *taosHashIterate(SHashObj *pHashObj, void *p);

src/util/src/hash.c

@@ -19,6 +19,9 @@
 #include "taosdef.h"
 
 #define EXT_SIZE 1024
+#define HASH_MAX_CAPACITY (1024 * 1024 * 16)
+#define HASH_DEFAULT_LOAD_FACTOR (0.75)
+#define HASH_INDEX(v, c) ((v) & ((c)-1))
 
 #define HASH_NEED_RESIZE(_h) ((_h)->size >= (_h)->capacity * HASH_DEFAULT_LOAD_FACTOR)
 
@@ -36,6 +39,32 @@
     DO_FREE_HASH_NODE(_n);      \
   } while (0);
 
+#define GET_HASH_NODE_KEY(_n)  ((char*)(_n) + sizeof(SHashNode) + (_n)->dataLen)
+#define GET_HASH_NODE_DATA(_n) ((char*)(_n) + sizeof(SHashNode))
+#define GET_HASH_PNODE(_n) ((SHashNode *)((char*)(_n) - sizeof(SHashNode)))
+
+typedef void (*_hash_free_fn_t)(void *param);
+
+typedef struct SHashEntry {
+  int32_t    num;      // number of elements in current entry
+  SRWLatch   latch;    // entry latch
+  SHashNode *next;
+} SHashEntry;
+
+typedef struct SHashObj {
+  SHashEntry    **hashList;
+  size_t          capacity;     // number of slots
+  size_t          size;         // number of elements in hash table
+  _hash_fn_t      hashFp;       // hash function
+  _hash_free_fn_t freeFp;       // hash node free callback function
+  _equal_fn_t     equalFp;       // equal function
+
+  SRWLatch        lock;         // read-write spin lock
+  SHashLockTypeE  type;         // lock type
+  bool            enableUpdate; // enable update
+  SArray         *pMemBlock;    // memory block allocated for SHashEntry
+} SHashObj;
+
 static FORCE_INLINE void __wr_lock(void *lock, int32_t type) {
   if (type == HASH_NO_LOCK) {
     return;
@@ -97,26 +126,26 @@ static FORCE_INLINE SHashNode *doSearchInEntryList(SHashObj *pHashObj, SHashEntr
 static void taosHashTableResize(SHashObj *pHashObj);
 
 /**
+ * allocate and initialize a hash node
+ *
  * @param key      key of object for hash, usually a null-terminated string
  * @param keyLen   length of key
- * @param pData    actually data. Requires a consecutive memory block, no pointer is allowed in pData.
- *                 Pointer copy causes memory access error.
+ * @param pData    data to be stored in hash node
  * @param dsize    size of data
- * @return         SHashNode
+ * @return         sHashNode
  */
 static SHashNode *doCreateHashNode(const void *key, size_t keyLen, const void *pData, size_t dsize, uint32_t hashVal);
 
 /**
- * Update the hash node
+ * update the hash node
  *
- * @param pNode   hash node
- * @param key     key for generate hash value
- * @param keyLen  key length
- * @param pData   actual data
- * @param dsize   size of actual data
- * @return        hash node
+ * @param pHashObj   hash table object
+ * @param pe         hash table entry to operate on
+ * @param prev       previous node
+ * @param pNode      the old node with requested key
+ * @param pNewNode   the new node with requested key
  */
-static FORCE_INLINE SHashNode *doUpdateHashNode(SHashObj *pHashObj, SHashEntry* pe, SHashNode* prev, SHashNode *pNode, SHashNode *pNewNode) {
+static FORCE_INLINE void doUpdateHashNode(SHashObj *pHashObj, SHashEntry* pe, SHashNode* prev, SHashNode *pNode, SHashNode *pNewNode) {
   assert(pNode->keyLen == pNewNode->keyLen);
 
   atomic_sub_fetch_32(&pNode->refCount, 1);
@@ -134,12 +163,10 @@ static FORCE_INLINE SHashNode *doUpdateHashNode(SHashObj *pHashObj, SHashEntry*
     pe->num++;
     atomic_add_fetch_64(&pHashObj->size, 1);
   }
-
-  return pNewNode;
 }
 
 /**
- * insert the hash node at the front of the linked list
+ * Insert the hash node at the front of the linked list
  *
  * @param pHashObj
  * @param pNode
@@ -155,16 +182,21 @@ static void pushfrontNodeInEntryList(SHashEntry *pEntry, SHashNode *pNode);
 static FORCE_INLINE bool taosHashTableEmpty(const SHashObj *pHashObj);
 
 /**
- * Initialize a hash table
+ * initialize a hash table
  *
- * @param capacity   Initial capacity of the hash table
- * @param fn         Hash function
- * @param update     Whether the hash table allows in place update
- * @param type       Whether the hash table has per entry lock
- * @return           Hash table object
+ * @param capacity   initial capacity of the hash table
+ * @param fn         hash function
+ * @param update     whether the hash table allows in place update
+ * @param type       whether the hash table has per entry lock
+ * @return           hash table object
  */
 SHashObj *taosHashInit(size_t capacity, _hash_fn_t fn, bool update, SHashLockTypeE type) {
   assert(fn != NULL);
+  if (fn == NULL) {
+    uError("hash table must have a valid hash function");
+    return NULL;
+  }
+
   if (capacity == 0) {
     capacity = 4;
   }
@@ -646,15 +678,15 @@ void taosHashTableResize(SHashObj *pHashObj) {
   SHashNode *pNode = NULL;
   SHashNode *pNext = NULL;
 
-  int32_t newSize = (int32_t)(pHashObj->capacity << 1u);
-  if (newSize > HASH_MAX_CAPACITY) {
+  int32_t newCapacity = (int32_t)(pHashObj->capacity << 1u);
+  if (newCapacity > HASH_MAX_CAPACITY) {
     //    uDebug("current capacity:%d, maximum capacity:%d, no resize applied due to limitation is reached",
     //           pHashObj->capacity, HASH_MAX_CAPACITY);
     return;
   }
 
   int64_t st = taosGetTimestampUs();
-  void *pNewEntryList = realloc(pHashObj->hashList, sizeof(void *) * newSize);
+  void *pNewEntryList = realloc(pHashObj->hashList, sizeof(void *) * newCapacity);
   if (pNewEntryList == NULL) {  // todo handle error
     //    uDebug("cache resize failed due to out of memory, capacity remain:%d", pHashObj->capacity);
     return;
@@ -662,7 +694,7 @@ void taosHashTableResize(SHashObj *pHashObj) {
 
   pHashObj->hashList = pNewEntryList;
 
-  size_t inc = newSize - pHashObj->capacity;
+  size_t inc = newCapacity - pHashObj->capacity;
   void * p = calloc(inc, sizeof(SHashEntry));
 
   for (int32_t i = 0; i < inc; ++i) {
@@ -671,7 +703,7 @@ void taosHashTableResize(SHashObj *pHashObj) {
 
   taosArrayPush(pHashObj->pMemBlock, &p);
 
-  pHashObj->capacity = newSize;
+  pHashObj->capacity = newCapacity;
   for (int32_t i = 0; i < pHashObj->capacity; ++i) {
     SHashEntry *pe = pHashObj->hashList[i];