kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
Changes persistance logic to store shards at the directory of closest
CDB. Previously we were storing all shards to directory of the CDB that
triggered indexing, it had its downsides.
For example, if you had two TUs coming from a different CDB but depending on the
same header foo.h, we will store the foo.h only for the first CDB, and it would
be missing for the second and we would never persist it since it was actually
present in the memory and persisted before.
This patch still stores only a single copy of a shard, but makes the directory a
function of the file name. So that the shard place will be unique even with
multiple CDBs accessing the file. This directory is determined as the first
directory containing a CDB in the file's parent directories, if no such
directory exists we make use of the home directory.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D64745
Files:
clang-tools-extra/clangd/index/Background.cpp
clang-tools-extra/clangd/index/Background.h
clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp
clang-tools-extra/clangd/index/BackgroundIndexLoader.h
clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
clang-tools-extra/clangd/test/Inputs/background-index/foo.cpp
clang-tools-extra/clangd/test/Inputs/background-index/foo.h
clang-tools-extra/clangd/test/Inputs/background-index/sub_dir/compile_flags.txt
clang-tools-extra/clangd/test/Inputs/background-index/sub_dir/foo.h
clang-tools-extra/clangd/test/background-index.test
Index: clang-tools-extra/clangd/test/background-index.test
===================================================================
--- clang-tools-extra/clangd/test/background-index.test
+++ clang-tools-extra/clangd/test/background-index.test
@@ -5,7 +5,8 @@
# RUN: rm -rf %t
# RUN: cp -r %S/Inputs/background-index %t
# Need to embed the correct temp path in the actual JSON-RPC requests.
-# RUN: sed -i -e "s|DIRECTORY|%t|" %t/*
+# RUN: sed -i -e "s|DIRECTORY|%t|" %t/definition.jsonrpc
+# RUN: sed -i -e "s|DIRECTORY|%t|" %t/compile_commands.json
# We're editing bar.cpp, which includes foo.h.
# foo() is declared in foo.h and defined in foo.cpp.
@@ -14,6 +15,7 @@
# Test that the index is writing files in the expected location.
# RUN: ls %t/.clangd/index/foo.cpp.*.idx
+# RUN: ls %t/sub_dir/.clangd/index/foo.h.*.idx
# Test the index is read from disk: delete code and restart clangd.
# RUN: rm %t/foo.cpp
Index: clang-tools-extra/clangd/test/Inputs/background-index/foo.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/test/Inputs/background-index/foo.h
@@ -1,4 +0,0 @@
-#ifndef FOO_H
-#define FOO_H
-int foo();
-#endif
Index: clang-tools-extra/clangd/test/Inputs/background-index/foo.cpp
===================================================================
--- clang-tools-extra/clangd/test/Inputs/background-index/foo.cpp
+++ clang-tools-extra/clangd/test/Inputs/background-index/foo.cpp
@@ -1,2 +1,2 @@
-#include "foo.h"
+#include "sub_dir/foo.h"
int foo() { return 42; }
Index: clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
===================================================================
--- clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
+++ clang-tools-extra/clangd/test/Inputs/background-index/definition.jsonrpc
@@ -18,7 +18,7 @@
"uri": "file://DIRECTORY/bar.cpp",
"languageId": "cpp",
"version": 1,
- "text": "#include \"foo.h\"\nint main(){\nreturn foo();\n}"
+ "text": "#include \"sub_dir/foo.h\"\nint main(){\nreturn foo();\n}"
}
}
}
Index: clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
===================================================================
--- clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
+++ clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp
@@ -6,9 +6,14 @@
//
//===----------------------------------------------------------------------===//
+#include "GlobalCompilationDatabase.h"
#include "Logger.h"
+#include "Path.h"
#include "index/Background.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBuffer.h"
@@ -119,11 +124,20 @@
class DiskBackedIndexStorageManager {
public:
DiskBackedIndexStorageManager()
- : IndexStorageMapMu(llvm::make_unique<std::mutex>()) {}
+ : IndexStorageMapMu(llvm::make_unique<std::mutex>()),
+ CDB(llvm::make_unique<DirectoryBasedGlobalCompilationDatabase>(
+ llvm::None)) {
+ llvm::SmallString<128> HomeDir;
+ llvm::sys::path::home_directory(HomeDir);
+ this->HomeDir = HomeDir.str().str();
+ }
// Creates or fetches to storage from cache for the specified CDB.
- BackgroundIndexStorage *operator()(llvm::StringRef CDBDirectory) {
+ BackgroundIndexStorage *operator()(PathRef File) {
std::lock_guard<std::mutex> Lock(*IndexStorageMapMu);
+ Path CDBDirectory = HomeDir;
+ if (auto PI = CDB->getProjectInfo(File))
+ CDBDirectory = PI->SourceRoot;
auto &IndexStorage = IndexStorageMap[CDBDirectory];
if (!IndexStorage)
IndexStorage = create(CDBDirectory);
@@ -131,14 +145,18 @@
}
private:
- std::unique_ptr<BackgroundIndexStorage> create(llvm::StringRef CDBDirectory) {
- if (CDBDirectory.empty())
- return llvm::make_unique<NullStorage>();
+ std::unique_ptr<BackgroundIndexStorage> create(PathRef CDBDirectory) {
+ assert(!CDBDirectory.empty() &&
+ "Tried to create storage for empty directory!");
return llvm::make_unique<DiskBackedIndexStorage>(CDBDirectory);
}
+ Path HomeDir;
+
llvm::StringMap<std::unique_ptr<BackgroundIndexStorage>> IndexStorageMap;
std::unique_ptr<std::mutex> IndexStorageMapMu;
+
+ std::unique_ptr<DirectoryBasedGlobalCompilationDatabase> CDB;
};
} // namespace
Index: clang-tools-extra/clangd/index/BackgroundIndexLoader.h
===================================================================
--- clang-tools-extra/clangd/index/BackgroundIndexLoader.h
+++ clang-tools-extra/clangd/index/BackgroundIndexLoader.h
@@ -29,10 +29,12 @@
class BackgroundIndexLoader {
public:
- BackgroundIndexLoader(llvm::vfs::FileSystem *FS) : FS(FS) {}
- /// Loads all shards for the TU \p MainFile from \p Storage. Returns false if
- /// any shards that are first seen for this TU are stale.
- bool load(PathRef MainFile, BackgroundIndexStorage *Storage);
+ BackgroundIndexLoader(llvm::vfs::FileSystem *FS,
+ BackgroundIndexStorage::Factory *IndexStorageFactory)
+ : FS(FS), IndexStorageFactory(IndexStorageFactory) {}
+ /// Loads all shards for the TU \p MainFile. Returns false if any shards that
+ /// are first seen for this TU are stale.
+ bool load(PathRef MainFile);
/// Consumes the loader and returns all shards.
std::vector<ShardInfo> takeShards() &&;
@@ -45,12 +47,14 @@
bool IsStale = false;
};
std::pair<const CachedShard &, /*WasCached=*/bool>
- loadShard(PathRef ShardIdentifier, BackgroundIndexStorage *Storage);
+ loadShard(PathRef ShardIdentifier);
// Cache for Storage lookups.
llvm::StringMap<CachedShard> LoadedShards;
llvm::vfs::FileSystem *FS;
+
+ BackgroundIndexStorage::Factory *IndexStorageFactory;
};
} // namespace clangd
Index: clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp
===================================================================
--- clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp
+++ clang-tools-extra/clangd/index/BackgroundIndexLoader.cpp
@@ -41,8 +41,7 @@
} // namespace
std::pair<const BackgroundIndexLoader::CachedShard &, /*WasCached=*/bool>
-BackgroundIndexLoader::loadShard(PathRef ShardIdentifier,
- BackgroundIndexStorage *Storage) {
+BackgroundIndexLoader::loadShard(PathRef ShardIdentifier) {
auto It = LoadedShards.try_emplace(ShardIdentifier);
CachedShard &CS = It.first->getValue();
if (!It.second)
@@ -50,6 +49,8 @@
ShardInfo &SI = CS.SI;
SI.AbsolutePath = ShardIdentifier;
+
+ BackgroundIndexStorage *Storage = (*IndexStorageFactory)(ShardIdentifier);
auto Shard = Storage->loadShard(ShardIdentifier);
if (!Shard || !Shard->Sources) {
vlog("Failed to load shard: {0}", ShardIdentifier);
@@ -77,13 +78,12 @@
return {CS, false};
}
-bool BackgroundIndexLoader::load(PathRef MainFile,
- BackgroundIndexStorage *Storage) {
+bool BackgroundIndexLoader::load(PathRef MainFile) {
bool IsStale = false;
// Following containers points to strings inside LoadedShards.
llvm::DenseSet<PathRef> InQueue;
std::queue<PathRef> ToVisit;
- auto LoadResult = loadShard(MainFile, Storage);
+ auto LoadResult = loadShard(MainFile);
const CachedShard &MainShard = LoadResult.first;
if (!LoadResult.second && MainShard.IsStale)
@@ -97,7 +97,7 @@
PathRef ShardIdentifier = ToVisit.front();
ToVisit.pop();
- auto LoadResult = loadShard(ShardIdentifier, Storage);
+ auto LoadResult = loadShard(ShardIdentifier);
const CachedShard &CS = LoadResult.first;
// Report the file as stale if it is the first time we see it.
// FIXME: We should still update staleness for main file, if it had errors.
Index: clang-tools-extra/clangd/index/Background.h
===================================================================
--- clang-tools-extra/clangd/index/Background.h
+++ clang-tools-extra/clangd/index/Background.h
@@ -50,14 +50,15 @@
virtual std::unique_ptr<IndexFileIn>
loadShard(llvm::StringRef ShardIdentifier) const = 0;
- // The factory provides storage for each CDB.
+ // The factory provides storage for each File.
// It keeps ownership of the storage instances, and should manage caching
// itself. Factory must be threadsafe and never returns nullptr.
- using Factory =
- llvm::unique_function<BackgroundIndexStorage *(llvm::StringRef)>;
+ using Factory = llvm::unique_function<BackgroundIndexStorage *(PathRef)>;
// Creates an Index Storage that saves shards into disk. Index storage uses
- // CDBDirectory + ".clangd/index/" as the folder to save shards.
+ // CDBDirectory + ".clangd/index/" as the folder to save shards. CDBDirectory
+ // is the first directory containing a CDB in parent directories of a file, or
+ // user's home directory if none was found, e.g. standard library headers.
static Factory createDiskBackedStorageFactory();
};
@@ -150,15 +151,14 @@
/// information on IndexStorage.
void update(llvm::StringRef MainFile, IndexFileIn Index,
const llvm::StringMap<ShardVersion> &ShardVersionsSnapshot,
- BackgroundIndexStorage *IndexStorage, bool HadErrors);
+ bool HadErrors);
// configuration
const FileSystemProvider &FSProvider;
const GlobalCompilationDatabase &CDB;
Context BackgroundContext;
- llvm::Error index(tooling::CompileCommand,
- BackgroundIndexStorage *IndexStorage);
+ llvm::Error index(tooling::CompileCommand);
FileSymbols IndexedSymbols;
BackgroundIndexRebuilder Rebuilder;
@@ -167,13 +167,12 @@
BackgroundIndexStorage::Factory IndexStorageFactory;
// Tries to load shards for the MainFiles.
- std::vector<std::pair<tooling::CompileCommand, BackgroundIndexStorage *>>
+ std::vector<tooling::CompileCommand>
loadProject(std::vector<std::string> MainFiles);
BackgroundQueue::Task
changedFilesTask(const std::vector<std::string> &ChangedFiles);
- BackgroundQueue::Task indexFileTask(tooling::CompileCommand Cmd,
- BackgroundIndexStorage *Storage);
+ BackgroundQueue::Task indexFileTask(tooling::CompileCommand Cmd);
// from lowest to highest priority
enum QueuePriority {
Index: clang-tools-extra/clangd/index/Background.cpp
===================================================================
--- clang-tools-extra/clangd/index/Background.cpp
+++ clang-tools-extra/clangd/index/Background.cpp
@@ -167,8 +167,8 @@
std::mt19937(std::random_device{}()));
std::vector<BackgroundQueue::Task> Tasks;
Tasks.reserve(NeedsReIndexing.size());
- for (auto &Elem : NeedsReIndexing)
- Tasks.push_back(indexFileTask(std::move(Elem.first), Elem.second));
+ for (auto &Cmd : NeedsReIndexing)
+ Tasks.push_back(indexFileTask(std::move(Cmd)));
Queue.append(std::move(Tasks));
});
@@ -178,13 +178,12 @@
}
BackgroundQueue::Task
-BackgroundIndex::indexFileTask(tooling::CompileCommand Cmd,
- BackgroundIndexStorage *Storage) {
- BackgroundQueue::Task T([this, Storage, Cmd] {
+BackgroundIndex::indexFileTask(tooling::CompileCommand Cmd) {
+ BackgroundQueue::Task T([this, Cmd] {
// We can't use llvm::StringRef here since we are going to
// move from Cmd during the call below.
const std::string FileName = Cmd.Filename;
- if (auto Error = index(std::move(Cmd), Storage))
+ if (auto Error = index(std::move(Cmd)))
elog("Indexing {0} failed: {1}", FileName, std::move(Error));
});
T.QueuePri = IndexFile;
@@ -197,7 +196,7 @@
void BackgroundIndex::update(
llvm::StringRef MainFile, IndexFileIn Index,
const llvm::StringMap<ShardVersion> &ShardVersionsSnapshot,
- BackgroundIndexStorage *IndexStorage, bool HadErrors) {
+ bool HadErrors) {
// Partition symbols/references into files.
struct File {
llvm::DenseSet<const Symbol *> Symbols;
@@ -281,22 +280,21 @@
// We need to store shards before updating the index, since the latter
// consumes slabs.
// FIXME: Also skip serializing the shard if it is already up-to-date.
- if (IndexStorage) {
- IndexFileOut Shard;
- Shard.Symbols = SS.get();
- Shard.Refs = RS.get();
- Shard.Relations = RelS.get();
- Shard.Sources = IG.get();
-
- // Only store command line hash for main files of the TU, since our
- // current model keeps only one version of a header file.
- if (Path == MainFile)
- Shard.Cmd = Index.Cmd.getPointer();
-
- if (auto Error = IndexStorage->storeShard(Path, Shard))
- elog("Failed to write background-index shard for file {0}: {1}", Path,
- std::move(Error));
- }
+ BackgroundIndexStorage *IndexStorage = IndexStorageFactory(Path);
+ IndexFileOut Shard;
+ Shard.Symbols = SS.get();
+ Shard.Refs = RS.get();
+ Shard.Relations = RelS.get();
+ Shard.Sources = IG.get();
+
+ // Only store command line hash for main files of the TU, since our
+ // current model keeps only one version of a header file.
+ if (Path == MainFile)
+ Shard.Cmd = Index.Cmd.getPointer();
+
+ if (auto Error = IndexStorage->storeShard(Path, Shard))
+ elog("Failed to write background-index shard for file {0}: {1}", Path,
+ std::move(Error));
{
std::lock_guard<std::mutex> Lock(ShardVersionsMu);
@@ -319,8 +317,7 @@
}
}
-llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd,
- BackgroundIndexStorage *IndexStorage) {
+llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd) {
trace::Span Tracer("BackgroundIndex");
SPAN_ATTACH(Tracer, "file", Cmd.Filename);
auto AbsolutePath = getAbsolutePath(Cmd);
@@ -414,8 +411,7 @@
for (auto &It : *Index.Sources)
It.second.Flags |= IncludeGraphNode::SourceFlag::HadErrors;
}
- update(AbsolutePath, std::move(Index), ShardVersionsSnapshot, IndexStorage,
- HadErrors);
+ update(AbsolutePath, std::move(Index), ShardVersionsSnapshot, HadErrors);
Rebuilder.indexedTU();
return llvm::Error::success();
@@ -423,13 +419,12 @@
// Goes over each changed file and loads them from index. Returns the list of
// TUs that had out-of-date/no shards.
-std::vector<std::pair<tooling::CompileCommand, BackgroundIndexStorage *>>
+std::vector<tooling::CompileCommand>
BackgroundIndex::loadProject(std::vector<std::string> MainFiles) {
- std::vector<std::pair<tooling::CompileCommand, BackgroundIndexStorage *>>
- NeedsReIndexing;
+ std::vector<tooling::CompileCommand> NeedsReIndexing;
auto FS = FSProvider.getFileSystem();
- BackgroundIndexLoader Loader(FS.get());
+ BackgroundIndexLoader Loader(FS.get(), &IndexStorageFactory);
Rebuilder.startLoading();
for (const auto &File : MainFiles) {
@@ -441,11 +436,10 @@
if (auto PI = CDB.getProjectInfo(File))
ProjectRoot = std::move(PI->SourceRoot);
- BackgroundIndexStorage *IndexStorage = IndexStorageFactory(ProjectRoot);
Path AbsPath = getAbsolutePath(*Cmd).str().str();
- if (!Loader.load(AbsPath, IndexStorage))
- NeedsReIndexing.push_back({std::move(*Cmd), IndexStorage});
+ if (!Loader.load(AbsPath))
+ NeedsReIndexing.push_back(std::move(*Cmd));
}
auto Shards = std::move(Loader).takeShards();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
