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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In https://reviews.llvm.org/D33009#751668, @zturner wrote:

> I'm still not entirely convinced.  At lunch we discussed doing this as a way to avoid having a `BitVector` at all.  But it turns out we need the `BitVector` regardless, because the `TypeDatabase` needs to know what records are valid, and it is decoupled from the visitor.  So that aspect doesn't really buy us anything.
>
> I think it comes down to access patterns.  90% or more of the types in a PDB are never going to be accessed.  You're looking for a symbol, it says the type of the symbol is 329082, what are the odds you're going to later access symbol 329075?  Pretty low by my estimation.  Type accesses will be sparse, and so while it will increase our cache hit ratio to scan the whole block up front, it doesn't necessarily seem like a win to me.  Most type accesses are going to come from un-examined chunks.


I think it avoids extra complexity that isn't necessary to get the same algorithmic guarantees. It lets us reduce the API surface area of TypeDatabase to avoid findPrevoius / find_last_in. I feel like data-structurey code needs to be as simple as humanly possible to make it easy to reason about the algorithmic properties. If we can get by with less `if`s, the better.



================
Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeDatabase.h:27
 public:
-  explicit TypeDatabase(uint32_t ExpectedSize);
+  enum InsertLocation { IL_FirstAvailable, IL_Append };
 
----------------
rnk wrote:
> 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?
What's the plan for IL_FirstAvailable?


https://reviews.llvm.org/D33009