[PATCH] D133715: [ADT] Add HashMappedTrie

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D133715#4029879 <https://reviews.llvm.org/D133715#4029879>, @steven_wu wrote:

> In D133715#4029841 <https://reviews.llvm.org/D133715#4029841>, @dexonsmith wrote:
>
>> In D133715#4029442 <https://reviews.llvm.org/D133715#4029442>, @steven_wu wrote:
>>
>>> Ping. All feedbacks are addressed.
>>>
>>> Additional notes: I dig a bit deeper into the benchmark from https://reviews.llvm.org/D132548 as it shows bad scaling during parallel insertion tests (stop scaling to 8 threads). That is actually caused by our ThreadSafeBumpPtrAllocator that currently takes a lock. We can improve it in future since our use case doesn't expect heavy parallel insertions. However, it is quite obvious we should tune to RootBits and SubTrieBits. Increasing RootBits can significantly decrease the contention. A better strategy might be something like starting with something like 10 bits, then 4 bits, 2 bits and 1 bit. Shrinking number of bits can lead to better memory usage since the slots usage in the deep nodes are very low.
>>
>> Could ThreadSafeBumpPtrAllocator could be made lock-free? I think at least it would be possible to implement one that only locked when a new allocation is needed, instead of every time the ptr is bumped as now. (I’ll think about it a bit.)
>>
>> Note that in the CAS use case it’s ideally true that most insertions are duplicates and don’t need to call the allocator at all. This is why we’ve been able to get away with a lock on each allocation.

Here's a sketch that I think mostly works?

  // Block for bump allocations.
  struct BumpBlock {
    std::atomic<char *> Ptr;
    std::atomic<BumpBlock *> Next;
    char Bytes[4084];
    BumpBlock() : Ptr{Bytes}, Next{nullptr} {}
  
    // Compute new "Next", try to bump if there's space, else return nullptr.
    void *tryAllocate(size_t N, size_t Align = 1);
  };
  
  // Tail-allocated data for "big" allocations.
  struct BumpSeparate {
    std::atomic<BumpSeparate *> Next;
    // tail-allocated data of appropriate and alignment, using malloc....
  };
  
  // Allocator.
  struct BumpAllocator {
    std::atomic<BumpBlock *> CurrentBlock;
    std::atomic<BumpSeparate *> LastAllocSeparately;
  
    // Delete everything since there's no ownership here...
    ~BumpAllocator();
  
    void *allocate(size_t N, size_t Align = 1) {
      if (N > 2048)
        return allocateSeparately(N, Align);
  
      BumpBlock *B = CurrentBlock;
      std::unique_ptr<BumpBlock> New;
      void *NewAlloc = nullptr;
      while (true) {
        if (LLVM_LIKELY(B))
          if (void *Alloc = B->tryAllocate(N, Align))
            return Alloc;
  
        if (!New) {
          New = new BumpBlock;
          NewAlloc = B->tryAllocate(N, Align);
          assert(NewAlloc && "Empty block doesn't have space!");
        }
        if (!CurrentBlock.compare_exchange_weak(B, New.get()))
          continue;
  
        // New was saved in CurrentBlock. Fix its "Next" pointer and release it
        // so it's not deallocated.
        New->Next = B;
        New.release();
        return NewAlloc;
      }
    }
  
  private:
    // Maintain a list of "big" allocations, similar to above.
    void *allocateSeparately(size_t N, size_t Align);
  };

Not saying it needs to block this review...

But having a fast concurrent BumpPtrAllocator would be independently useful, and I'd suggest optimizing the allocator before bloating the default trie size.

> Yes, the `FIXME` for a ThreadSafeBumpPtrAllocator is still there. Currently, I don't think it is urgent to fix. It is not expected to have someone to use Trie as a high performance thread safe set/map.

The immediate use case is as a high performance thread-safe data store. In the CAS use case we're expecting a lot of duplicate insertions which don't happen to hit the allocator, but I'm not sure that's really been checked/measured.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133715