[PATCH] D33009: [CodeView] Add an amortized O(1) random access type visitor

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I think you probably want to go to the simpler strategy we described at lunch, which looks more like:

- Maintain the KnownOffsets map from TypeIndex to offset, where all offsets start off unknown. You could use a sentinel value (~0U) or a separate BitVector to track if the offset is known.
- If an offset is unknown, binary search the IndexToOffset array to find the start and end offsets of that range of records.
- Scan that range of records and update KnownOffsets for all records in that range.

This avoids bad worst case behavior that might come up if the user scans forwards or backwards through type indices. The difference between this algorithm and what you have is that you're trying to avoid scanning the complete range, but that means you have to do scans through the BitVector to try to resume scanning where you left off. That isn't strictly necessary to get the fast random access that we're looking for.



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/RandomAccessTypeVisitor.h:71
+
+  // Visited records get automatically added to the type database.
+  TypeDatabase Database;
----------------
You should try to use the Doxygen `///` comments when it makes sense here and below. I don't consume the Doxygen, but there are people who do.


================
Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeDatabase.h:27
 public:
-  explicit TypeDatabase(uint32_t ExpectedSize);
+  enum InsertLocation { IL_FirstAvailable, IL_Append };
 
----------------
No caller uses IL_FirstAvailable. What's the use case? Do we need this overload at all, or can we standardize on calling recordType with TypeIndex?


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeDatabase.cpp:156
+void TypeDatabase::grow() {
+  TypeRecords.resize(TypeRecords.size() + 1);
+  CVUDTNames.resize(CVUDTNames.size() + 1);
----------------
This will most likely be O(n^2) because it bypasses the growth multiplier in vector push_back.


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeDatabase.cpp:165
+  uint32_t ZI = PriorTo.toArrayIndex();
+  int N = ValidRecords.find_prev(ZI);
+  if (N == -1)
----------------
This is linear, it might scan to the beginning of the BitVector without stopping at the beginning of the range.


https://reviews.llvm.org/D33009