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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
