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

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Finally reached the end. Phew!

I think there is some substantial complexity here, and you'll probably want to think about how to get some test coverage of the implementation.

One edge case I didn't quite reason through is trimming down to size zero might trigger special case sentinel checks being invalid for instance.



================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:283-290
+  /// Remove N Elements from the end.
+  void trim(size_t Elements) {
+    // TODO: Figure out whether we need to actually destroy the objects that are
+    // going to be trimmed -- determine whether assuming that trivially
+    // destructible objects are OK to discard, from the XRay implementation.
+    DCHECK_LE(Elements, Size);
+    Size -= Elements;
----------------
You want to reuse the allocated chunks when growing the list rather than allocate new ones right?


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:295-296
+  Iterator end() const { return Iterator(Tail, Size); }
+  Iterator cbegin() const { return Iterator(Head, 0); }
+  Iterator cend() const { return Iterator(Tail, Size); }
+};
----------------
Isn't this a peculiar that these don't return a const type? Will this be a problem in practice?


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:299-300
+
+template <class T, size_t N>
+typename Array<T, N>::Chunk Array<T, N>::SentinelChunk;
+
----------------
Can you explain this statement please? My template foo is a bit rusty. Is this a templated member definition and which version of the standard does it require? LLVM is still on 11 IIUC.


https://reviews.llvm.org/D45756