[PATCH] D48653: [XRay][compiler-rt] xray::Array Freelist and Iterator Updates

[Keith via Phabricator via llvm-commits]

kpw accepted this revision.
kpw added a comment.
This revision is now accepted and ready to land.

Seems like half of this change is DCHECKS. Much better coverage! Was that just a result of your strategy for diagnosing what actually caused the problems and leaving those in?



================
Comment at: compiler-rt/lib/xray/tests/unit/function_call_trie_test.cc:177
+  for (int i = 0; i < 32; ++i) {
+    EXPECT_EQ(F->FId, i + 1);
+    if (F->Callees.empty() && i != 31)
----------------
No EXPECT statement for the time tracking?


================
Comment at: compiler-rt/lib/xray/tests/unit/segmented_array_test.cc:129-130
   // Trim one chunk's elements worth.
-  data.trim(ChunkX2 / 2);
-  ASSERT_EQ(data.size(), ChunkX2 / 2);
+  Data.trim(ChunkX2 / 2);
+  ASSERT_EQ(Data.size(), ChunkX2 / 2);
+
----------------
You can replace "ChunkX2 / 2" with the equivalent "Chunk" here and below.


================
Comment at: compiler-rt/lib/xray/tests/unit/segmented_array_test.cc:150
   for (auto i = ChunkX2; i > 0u; --i) {
-    data.Append({static_cast<s64>(i), static_cast<s64>(i)});
+    Data.Append({static_cast<s64>(i), static_cast<s64>(i)});
+  }
----------------
Why Append here and AppendEmplace above?


================
Comment at: compiler-rt/lib/xray/xray_allocator.h:153
       // that at once.
-      InternalFree(C->Blocks[0].Data);
+      InternalFree(C->BackingStore);
       auto Prev = C->Prev;
----------------
So trim of segmented array was free-ing the "block" with the wrong allocator when trimming the first element?


================
Comment at: compiler-rt/lib/xray/xray_function_call_trie.h:367
       // space. Maybe warn or report once?
-      DFSStack.Append(NodeAndParent{Root, NewRoot});
+      DFSStack.AppendEmplace(Root, NewRoot);
       while (!DFSStack.empty()) {
----------------
Should the NewRoots point back to the old roots as a parent? Don't we want to keep this isolated?


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:44
   static constexpr size_t ChunkSize = N;
-  static constexpr size_t AllocatorChunkSize = sizeof(T) * ChunkSize;
+  static constexpr size_t ElementStorageSize = next_pow2(sizeof(T));
+  static constexpr size_t AllocatorChunkSize = ElementStorageSize * ChunkSize;
----------------
I assume this is all alignment related?


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:171
+      auto PreviousOffset = Offset--;
+      if (PreviousOffset != Size && PreviousOffset % N == 0) {
+        DCHECK_NE(C->Prev, &SentinelChunk);
----------------
This size check is applicable when decrementing end()?


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:204-205
+      auto RelOff = Offset % N;
+      return *reinterpret_cast<U *>(reinterpret_cast<char *>(C->Block.Data) +
+                                    (RelOff * ElementStorageSize));
     }
----------------
Lol. Checks out.


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:289-290
     }
-    auto Position = reinterpret_cast<T *>(C->Block.Data) + Offset;
-    return *Position;
+    return *reinterpret_cast<T *>(reinterpret_cast<char *>(C->Block.Data) +
+                                  (Offset * ElementStorageSize));
   }
----------------
This is common enough you might want to put it into an always inlined private member function. Maybe currentElementData()


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:303
+    auto It = end();
+    --It;
+    return *It;
----------------
I would comment operator-- with the fact that it must not mutate the iterator. It's counterintuitive that it's non-destructive while ++ advances state.


================
Comment at: compiler-rt/lib/xray/xray_segmented_array.h:331
+    for (auto ChunksToTrim =
+             nearest_boundary(OldSize - nearest_boundary(Size, N), N) / N;
+         ChunksToTrim > 0; --ChunksToTrim) {
----------------
This feels like it probably simplifies to an equivalent expression.


https://reviews.llvm.org/D48653