5 files changed, 35 insertions, 14 deletions
diff --git a/clang-tools-extra/clangd/index/Index.cpp b/clang-tools-extra/clangd/index/Index.cpp
index f609056f639..95ecb6b18ec 100644
--- a/clang-tools-extra/clangd/index/Index.cpp
+++ b/clang-tools-extra/clangd/index/Index.cpp
@@ -38,9 +38,17 @@ SymbolSlab::const_iterator SymbolSlab::find(const SymbolID &SymID) const {
 
 void SymbolSlab::freeze() { Frozen = true; }
 
-void SymbolSlab::insert(Symbol S) {
+void SymbolSlab::insert(const Symbol &S) {
   assert(!Frozen && "Can't insert a symbol after the slab has been frozen!");
-  Symbols[S.ID] = std::move(S);
+  auto ItInserted = Symbols.try_emplace(S.ID, S);
+  if (!ItInserted.second)
+    return;
+  auto &Sym = ItInserted.first->second;
+
+  // We inserted a new symbol, so copy the underlying data.
+  intern(Sym.Name);
+  intern(Sym.Scope);
+  intern(Sym.CanonicalDeclaration.FilePath);
 }
 
 } // namespace clangd
diff --git a/clang-tools-extra/clangd/index/Index.h b/clang-tools-extra/clangd/index/Index.h
index 60aaebaeaf1..f0414bf7b4c 100644
--- a/clang-tools-extra/clangd/index/Index.h
+++ b/clang-tools-extra/clangd/index/Index.h
@@ -15,6 +15,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
 #include <array>
 #include <string>
 
@@ -23,7 +24,7 @@ namespace clangd {
 
 struct SymbolLocation {
   // The absolute path of the source file where a symbol occurs.
-  std::string FilePath;
+  llvm::StringRef FilePath;
   // The 0-based offset to the first character of the symbol from the beginning
   // of the source file.
   unsigned StartOffset;
@@ -71,16 +72,19 @@ void operator>>(llvm::StringRef HexStr, SymbolID &ID);
 
 // The class presents a C++ symbol, e.g. class, function.
 //
-// FIXME: instead of having own copy fields for each symbol, we can share
-// storage from SymbolSlab.
+// WARNING: Symbols do not own much of their underlying data - typically strings
+// are owned by a SymbolSlab. They should be treated as non-owning references.
+// Copies are shallow.
+// When adding new unowned data fields to Symbol, remember to update
+// SymbolSlab::insert to copy them to the slab's storage.
 struct Symbol {
   // The ID of the symbol.
   SymbolID ID;
   // The unqualified name of the symbol, e.g. "bar" (for "n1::n2::bar").
-  std::string Name;
+  llvm::StringRef Name;
   // The scope (e.g. namespace) of the symbol, e.g. "n1::n2" (for
   // "n1::n2::bar").
-  std::string Scope;
+  llvm::StringRef Scope;
   // The symbol information, like symbol kind.
   index::SymbolInfo SymInfo;
   // The location of the canonical declaration of the symbol.
@@ -100,9 +104,6 @@ struct Symbol {
 
 // A symbol container that stores a set of symbols. The container will maintain
 // the lifetime of the symbols.
-//
-// FIXME: Use a space-efficient implementation, a lot of Symbol fields could
-// share the same storage.
 class SymbolSlab {
 public:
   using const_iterator = llvm::DenseMap<SymbolID, Symbol>::const_iterator;
@@ -117,11 +118,21 @@ public:
   // operation is irreversible.
   void freeze();
 
-  void insert(Symbol S);
+  // Adds the symbol to this slab.
+  // This is a deep copy: underlying strings will be owned by the slab.
+  void insert(const Symbol& S);
 
 private:
+  // Replaces S with a reference to the same string, owned by this slab.
+  void intern(llvm::StringRef &S) {
+    S = S.empty() ? llvm::StringRef() : Strings.insert(S).first->getKey();
+  }
+
   bool Frozen = false;
 
+  // Intern table for strings. Not StringPool as we don't refcount, just insert.
+  // We use BumpPtrAllocator to avoid lots of tiny allocations for nodes.
+  llvm::StringSet<llvm::BumpPtrAllocator> Strings;
   llvm::DenseMap<SymbolID, Symbol> Symbols;
 };
 
diff --git a/clang-tools-extra/unittests/clangd/FileIndexTests.cpp b/clang-tools-extra/unittests/clangd/FileIndexTests.cpp
index 2371d7dc79d..c7bce7b5e79 100644
--- a/clang-tools-extra/unittests/clangd/FileIndexTests.cpp
+++ b/clang-tools-extra/unittests/clangd/FileIndexTests.cpp
@@ -93,7 +93,8 @@ std::vector<std::string> match(const SymbolIndex &I,
   std::vector<std::string> Matches;
   auto Ctx = Context::empty();
   I.fuzzyFind(Ctx, Req, [&](const Symbol &Sym) {
-    Matches.push_back(Sym.Scope + (Sym.Scope.empty() ? "" : "::") + Sym.Name);
+    Matches.push_back(
+        (Sym.Scope + (Sym.Scope.empty() ? "" : "::") + Sym.Name).str());
   });
   return Matches;
 }
diff --git a/clang-tools-extra/unittests/clangd/IndexTests.cpp b/clang-tools-extra/unittests/clangd/IndexTests.cpp
index a556424c0bd..0a4f31a7f1b 100644
--- a/clang-tools-extra/unittests/clangd/IndexTests.cpp
+++ b/clang-tools-extra/unittests/clangd/IndexTests.cpp
@@ -75,7 +75,8 @@ std::vector<std::string> match(const SymbolIndex &I,
   std::vector<std::string> Matches;
   auto Ctx = Context::empty();
   I.fuzzyFind(Ctx, Req, [&](const Symbol &Sym) {
-    Matches.push_back(Sym.Scope + (Sym.Scope.empty() ? "" : "::") + Sym.Name);
+    Matches.push_back(
+        (Sym.Scope + (Sym.Scope.empty() ? "" : "::") + Sym.Name).str());
   });
   return Matches;
 }
diff --git a/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp b/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
index aeaffefb7ae..acbecbae7db 100644
--- a/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
+++ b/clang-tools-extra/unittests/clangd/SymbolCollectorTests.cpp
@@ -33,7 +33,7 @@ using testing::UnorderedElementsAre;
 // GMock helpers for matching Symbol.
 MATCHER_P(QName, Name, "") {
   return (arg.second.Scope + (arg.second.Scope.empty() ? "" : "::") +
-          arg.second.Name) == Name;
+          arg.second.Name).str() == Name;
 }
 
 namespace clang {