[PATCH] D125979: [ADT] add ConcurrentHashTable class.
Alexey Lapshin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 31 07:00:18 PDT 2022
avl added inline comments.
================
Comment at: llvm/include/llvm/ADT/ConcurrentHashTable.h:117-118
+ else if (CurSlabNum > MaxNumberOfSlabs)
+ reportWarning("Too many slabs. Consider increasing initial size or "
+ "changing hash function.");
+
----------------
dblaikie wrote:
> It's pretty weird to have warnings coming out of data structures. @MaskRay would it be reasonable to remove these/replace them with some other tool if we're promoting this to a more general purpose data structure rather than something more special-case in lld?
Probably, instead of warning we can introduce method:
```
bool IsInGoodState () const {
return CurSlabNum <= MaxNumberOfSlabs && !hasFullSlab;
}
```
the consumer could call this method somewhere and inform the user:
```
if (!HashTable.IsInGoodState()) {
outs() << "Hash table is in suboptimal state";
}
```
?
================
Comment at: llvm/include/llvm/ADT/ConcurrentHashTable.h:315-316
+///
+/// It is recommended to use ConcurrentHashTableExtAlloc if sizeof(KeyDataTy) >
+/// sizeof(pointer).
+///
----------------
dblaikie wrote:
> This should probably be provided via specialization, rather than as a separately named type?
>
> I guess it has different requirements for the type, by the looks of it? That's curious/maybe enough to warrant a separate name, I guess, though I'm not 100% sure.
They have different interface:
```
std::pair<KeyDataTy, bool> insert(KeyDataTy NewData)
```
and
```
std::pair<KeyDataTy *, bool> insert(AllocatorTy &Allocator, KeyTy Key)
```
Implementing this through the specialization looks to be more complex and it seems does not have benefits comparing to the variant with separately named types?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125979/new/
https://reviews.llvm.org/D125979
More information about the llvm-commits
mailing list