[PATCH] D125979: [ADT] add LockFreeDataPool class.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

> LockFreeDataPool

The name is not descriptive. It misses the important words about "hash map". ConcurrentHashMap may be a better name.

The slab system appears to be the modification to `lld/COFF/DebugTypes.cpp`. There is a hard limit of 6 slabs, and `report_fatal_error` may eventually be called.
Then why is `reportWarning` needed?

Does your application have a reasonable way estimating the capacity? Does it help to use HyperLogLog?
If we want to prevent a crash in a pathological case where the estimated capacity is insufficient and causes the concurrent hash map to crash, we can use something like `lld::safeLldMain` (`llvm::CrashRecoveryContext`).



================
Comment at: llvm/include/llvm/ADT/LockFreeDataPool.h:26
+///
+/// The implementation of hash-table is copied(with modifications)
+/// from lld/COFF/DebugTypes.cpp
----------------
I think mentioning the origin in the summary is sufficient, no need to keep it in the comment.


================
Comment at: llvm/include/llvm/ADT/LockFreeDataPool.h:58
+    TableSize = NextPowerOf2(InitialSize);
+    TableSize = std::max(TableSize, (size_t)4096);
+    HashMask = TableSize - 1;
----------------
Why so large?


================
Comment at: llvm/include/llvm/ADT/LockFreeDataPool.h:159
+      // - cell has non-matching key: hash collision, probe next cell
+      KeyDataTy *Candidate = CurSlab->Table[Idx];
+      if (Candidate == nullptr) {
----------------
relaxed load


================
Comment at: llvm/include/llvm/ADT/LockFreeDataPool.h:229
+  size_t HashMask = 0;
+  const size_t MaxBucketLength = 100;
+  const size_t MaxNumberOfSlabs = 3;
----------------
MaxBucketLength needs an explanation.


================
Comment at: llvm/include/llvm/ADT/LockFreeDataPool.h:236
+
+  std::function<void(const char *message)> WarningHandler;
+  bool WarningIsReported = false;
----------------
Is this useful? See my main comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125979