[PATCH] D45756: [XRay][profiler] Part 1: XRay Allocator and Array Implementations

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Haven't looked at segmented array yet, but I had a good look at the allocator. I think I caught one serious bug, and the rest is just suggestions.



================
Comment at: compiler-rt/lib/xray/CMakeLists.txt:21-24
+set(x86_64_SOURCES
+    xray_x86_64.cc
+    xray_trampoline_x86_64.S)
+
----------------
Didn't like Martin's attempt to alphabetize? ;)


================
Comment at: compiler-rt/lib/xray/tests/unit/allocator_test.cc:30
+  auto B = A.Allocate();
+  (void)B;
+}
----------------
Expect not null? I don't grok this syntax.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:35
+///
+template <size_t N> struct Allocator {
+  // Each Block will have a fixed size, dependent on N. The allocator will hand
----------------
Can you document the template parameter in the class level comment? Minimum block size?


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:36-38
+  // Each Block will have a fixed size, dependent on N. The allocator will hand
+  // out individual blocks identified by the Data pointer, which is associated
+  // with a block.
----------------
If you comment the template parameter in the class comment. You can replace this with something to the effect of "A block is a a piece of memory handed out by the Allocator"


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:52
+  // bidirectional.
+  struct BlockChain {
+    static_assert(kCacheLineSize % sizeof(void *) == 0,
----------------
This name collides with the cryptographic immutable content-address data structure. I would try to avoid that. BlockListNode?


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:57-59
+    // This size represents the most number of pointers we can fit in a cache
+    // line, along with the next and previous pointers, and the bitmap to
+    // indicate which blocks have been handed out, and which ones are available.
----------------
Could you make this more clear? Maybe even name it BlockPtrsPerCacheLine instead of Size.  I had to read it a few times.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:66
+    // might store up to 16 pointers.
+    static constexpr auto Size = (kCacheLineSize / sizeof(Block)) - 3;
+    static_assert(Size <= 16, "Need to support larger than 16 ");
----------------
sizeof(Block*)? Should be equivalent since Block just wraps void*, but could be more clear. I see this type of punning show up in a few other places, so maybe it's alright to be consistent.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:71-73
+    BlockChain *Prev = nullptr;
+    BlockChain *Next = nullptr;
+    u16 UsedBits = 0;
----------------
If you're punting on the freelist you might be able to squeeze some more data into here.

UsedBits can actually be disposed of if you rely on always advancing the Chain when Counter % BlockChain::Size == 0 instead of wrap around detection.

Similarly, *Next is never actually used.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:86
+  SpinMutex Mutex{};
+  BlockChain *Chain = &NullLink;
+  size_t Counter = 0;
----------------
Maybe call this Tail to communicate that it is updated to refer to the node to pull new blocks from and not the head?


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:101-103
+    NewChain->Prev = Chain;
+    NewChain->Next = &NullLink;
+    return NewChain;
----------------
Did you consider having this function accept the tail of the list? Then it could write forward links in addition to backward links. It also seems fishy (at least if you want to do a free list) that this always points back to the Chain member.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:123
+      // as the Block objects.
+      auto NewChain = NewChainLink();
+
----------------
I prefer 'auto*' in cases like this.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:130
+      auto B = NewChain->Blocks[ChainOffset];
+      Chain->Next = NewChain;
+      Chain = NewChain;
----------------
NullLink is static right? This seems messy, although I know you don't intend to install multiple allocators.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:138
+    // link can be used.
+    if ((Chain->UsedBits | BitMask) == 0) {
+      auto B = Chain->Blocks[ChainOffset];
----------------
Pretty sure you meant & instead of |.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:145-147
+    // At this point we know that we've wrapped around, and the current chain no
+    // longer has free blocks. We then allocate a new chain and make that the
+    // current chain.
----------------
DCHECK that the ChainOffset == 0 and BitMask == 1?


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:159
+    // We need to deallocate all the blocks, including the chain links.
+    for (auto C = Chain; C != &NullLink;) {
+      // We know that the data block is a large contiguous page, we deallocate
----------------
auto* C please.


https://reviews.llvm.org/D45756