[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/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;
----------------
kpw wrote:
> You want to reuse the allocated chunks when growing the list rather than allocate new ones right?
Yes, that's correct. Good call, fixed.


================
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); }
+};
----------------
kpw wrote:
> Isn't this a peculiar that these don't return a const type? Will this be a problem in practice?
In practice, for our use-case, it shouldn't be a problem, although ideally the type of the Iterator should be different for the non-const and const case. For us here, it shouldn't matter (yet). The way to fix this would be to make the Iterator type itself a template, so that we can determine what the result of the dereference and arrow operators are, whether they are `const T` or `T`.

I've made the change to make it more "technically correct", which is the best kind of correct. :)


================
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;
+
----------------
kpw wrote:
> 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.
So, what happens here is we're telling the compiler that if the type `Array<T, N>` is ever instantiated in a translation unit, that we're going to get storage for the static data member. This is because `SentinelChunk` is a static data member of the `Array<T,N>` type, and it needs static (program duration) storage to be defined across translation units. This is how you spell this in C++98, AFAICT.


https://reviews.llvm.org/D45756