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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: compiler-rt/lib/xray/CMakeLists.txt:21-24
+set(x86_64_SOURCES
+    xray_x86_64.cc
+    xray_trampoline_x86_64.S)
+
----------------
kpw wrote:
> Didn't like Martin's attempt to alphabetize? ;)
Oh, I just thought it was harder to track which files where in which bucket by having them all in the same line. Visually spelling these out this way allows me to determine whether there are missing files quickly.


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


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:71-73
+    BlockChain *Prev = nullptr;
+    BlockChain *Next = nullptr;
+    u16 UsedBits = 0;
----------------
kpw wrote:
> 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.
Good call! Yeah, I kind-of stopped short of cleaning this up further.

Definitely something to think about if we ever need the freelist, but not really required at this time.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:101-103
+    NewChain->Prev = Chain;
+    NewChain->Next = &NullLink;
+    return NewChain;
----------------
kpw wrote:
> 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.
It could be, but now that we're not actually using the forward links (thanks for catching that), we can simplify this a lot now.


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:130
+      auto B = NewChain->Blocks[ChainOffset];
+      Chain->Next = NewChain;
+      Chain = NewChain;
----------------
kpw wrote:
> NullLink is static right? This seems messy, although I know you don't intend to install multiple allocators.
Yeah, NullLink is used as a sentinel value, which simplifies a lot of the operations for determining whether it's the end. It could as easily have been a nullptr though.


https://reviews.llvm.org/D45756