[PATCH] D58880: [WIP] [Looking for API feedback] [clangd] Type hierarchy subtypes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> My understanding is that the storage space taken up for Refs is currently 8 bytes per Ref (4 each for the start and end positions), plus filename strings which are deduplicated across all refs. If we add a SymbolID, that adds an additional 8 bytes to each Ref. Given that Refs are numerous, and most of them won't use the SymbolID, that seems wasteful.

I see.  This is a very good observation, we didn't consider increase in memory usage when we discussed your patch.  Sorry about that!  **We discussed your patch again and we agree with the approach of adding a RelationsSlab.**

Here are the options we considered, and the trade-offs we identified.

Option 1: Add SymbolID to Ref.  Advantage: simpler API, fewer types in the API.  Disadvantage: increased memory usage, as you noted.

Option 2: Add Relation struct, but store them in RefSlab, using a compact representation that skips storing SymbolID where it is not present.  Advantages: we avoid adding a type that's almost an exact duplicate of RefSlab, we don't unnecessarily increase memory usage.  Disadvantages: tricky code to avoid storing SymbolID, and RefSlab now stores both Refs and Relations, which is confusing from the API point of view.

Option 3 (what your patch implements): Add Relation struct and RelationSlab.  Advantage: API matches the data model, we don't unnecessarily increase memory usage.  Disadvantage: have to plumb the new slab everywhere in the code.

Option 4: Add a Relation struct.  Change RefSlab to a template so that it can store either Refs, or Relations.  Advantage: API matches the data model, we don't unnecessarily increase memory usage, we avoid duplicating RefSlab type.  Disadvantage: possibly complex template in a in important clangd API, we avoided duplicating at most 100 lines, but we still have to plumb the new slab everywhere in the code.



================
Comment at: clang-tools-extra/clangd/index/FileIndex.h:125
 
+using SlabTuple = std::tuple<SymbolSlab, RefSlab, RelationSlab>;
+
----------------
AllSlabs?  And even better, a struct?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58880/new/

https://reviews.llvm.org/D58880