[PATCH] D38433: Introduce a specialized data structure to be used in a subsequent change

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Have you considered building a `ChunkedVector` instead of a `ChunkedList`? Specifically, there is a great trick where you use a single index with the low bits being an index into the chunk and the high bits being an index into a vector of pointers. It has many of the benefits you list and is a bit simpler I think. It also supports essentially the entire vector API if desired. Both bi-directional and even random access are reasonably efficient. Good locality, etc.

The only downside I can see is that is is possible to implement insertion into the middle of `ChunkedList` with bounded complexity, but with `ChunkedVector` it would still be linear. However, you don't seem to have implemented arbitrary insertion and so I'm hoping that isn't necessary.

If you go the route of `ChunkedVector`, I'd also suggest thinking about what a useful SSO would look like... At the least an SmallVector for the pointers to chunks, but maybe also the ability to put the first chunk inline? Might make it too big in essentially all cases.

My guess is that `SmallVector<std::array<T, 4096/sizeof(T)>, N>` (for some small `N`) is going to be hard to beat.



================
Comment at: include/llvm/ADT/ChunkedList.h:9-24
+//
+// A chunked list is a unidirectional linked list where each node in the list
+// contains an array of \c N values.
+//
+// Pros:
+//
+// - Constant (and low, depending on \c N) memory overhead: the amount of memory
----------------
Move to a doxygen comment on the data structure?


================
Comment at: include/llvm/ADT/ChunkedList.h:62-65
+    End = Other.End;
+    Other.End = nullptr;
+    Capacity = Other.Capacity;
+    Other.Capacity = nullptr;
----------------
swap?


================
Comment at: include/llvm/ADT/ChunkedList.h:74-80
+    // Implementation Note:
+    //
+    // Iterators traverse the ChunkedList in LIFO order.  They maintain a
+    // pointer to the current element, \c Current; and to the first element in
+    // the chunk the current element belongs to, \c First.  Once we've hit \c
+    // First, we use the known offset of \c First within \c Chunk to get to the
+    // containing \c Chunk instance, and retreive the previous chunk.
----------------
The location of this comment doesn't makee it easy to find and looks like it pertains to a using declaration which doesn't make much sense.

I'd suggest:

1) having a (doxygen) API comment
2) sinking the implementation details to be attached to code in question. Either to the members or to the increment logic where this is implemented


https://reviews.llvm.org/D38433