[PATCH] D132455: [ADT] add ConcurrentHashtable class.

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

In D132455#4106255 <https://reviews.llvm.org/D132455#4106255>, @avl wrote:

> ping.
>
> @JDevlieghere @aprantl Do you think it is better to move this ConcurrentHashtable into the DWARFLinkerParallel folder?

I'm fine either way. If someone has concerns about the implementation, it's probably less contentious to land it in the DWARFLinkerParallel first, but so far I've not heard any objections. If we do want to use this from LLDB, then we'll need it to be in ADT eventually, though I'd like to do a comparison with Steven's `HashMappedTrie` for LLDB's real world usage.

I read through the code and most of it makes sense to me, but I wouldn't mind if someone that deal with data structures on a daily basis has another look. I'll mark this as accepted but would ask you to let it sit here for a few more days for others to take a look.



================
Comment at: llvm/include/llvm/ADT/ConcurrentHashtable.h:113-114
+
+    size_t UINT64_BitsNum = sizeof(uint64_t) * 8;
+    size_t UINT32_BitsNum = sizeof(uint32_t) * 8;
+
----------------



================
Comment at: llvm/include/llvm/ADT/ConcurrentHashtable.h:222
+    llvm_unreachable("Insertion error.");
+    return std::pair<KeyDataTy *, bool>();
+  }
----------------



================
Comment at: llvm/include/llvm/ADT/ConcurrentHashtable.h:297
+    assert((CurBucket.Size > 0) && "Uninitialised bucket");
+    if (CurBucket.NumberOfEntries < CurBucket.Size * 0.9)
+      return;
----------------
What's the benefit of rehashing at 90% capacity? It seems like this is going to always leave a few empty slots on the table? I understand you always need to have one slot because you rehash after insertion, but it seems like you could rehash here rehash when you've exhausted the bucket? 


================
Comment at: llvm/include/llvm/ADT/ConcurrentHashtable.h:308
+
+    // Store old entries&hashes arrays.
+    HashesPtr SrcHashes = CurBucket.Hashes;
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132455/new/

https://reviews.llvm.org/D132455