[clang-tools-extra] r346942 - Revert "Address comments."
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 15 02:34:35 PST 2018
Author: kadircet
Date: Thu Nov 15 02:34:35 2018
New Revision: 346942
URL: http://llvm.org/viewvc/llvm-project?rev=346942&view=rev
Log:
Revert "Address comments."
This reverts commit b43c4d1c731e07172a382567f3146b3c461c5b69.
Modified:
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/clangd/index/Background.h
clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=346942&r1=346941&r2=346942&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Thu Nov 15 02:34:35 2018
@@ -48,49 +48,54 @@ static Optional<BackgroundIndex::FileDig
return digest(Content);
}
-std::string getShardPathFromFilePath(llvm::StringRef ShardRoot,
- llvm::StringRef FilePath) {
- llvm::SmallString<128> ShardRootSS(ShardRoot);
- sys::path::append(ShardRootSS, sys::path::filename(FilePath) +
- toHex(digest(FilePath)) + ".idx");
- return ShardRoot.str();
+llvm::SmallString<128>
+getShardPathFromFilePath(llvm::SmallString<128> ShardRoot,
+ llvm::StringRef FilePath) {
+ sys::path::append(ShardRoot, sys::path::filename(FilePath) +
+ toHex(digest(FilePath)) + ".idx");
+ return ShardRoot;
}
// Uses disk as a storage for index shards. Creates a directory called
-// ".clangd-index/" under the path provided during construction.
+// ".clangd-index/" under the path provided during initialize.
+// Note: Requires initialize to be called before storing or retrieval.
// This class is thread-safe.
class DiskBackedIndexStorage : public BackgroundIndexStorage {
- std::string DiskShardRoot;
+ llvm::SmallString<128> DiskShardRoot;
public:
- llvm::Expected<IndexFileIn> loadShard(llvm::StringRef ShardIdentifier) const {
- const std::string ShardPath =
- getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
+ llvm::Expected<IndexFileIn>
+ retrieveShard(llvm::StringRef ShardIdentifier) const override {
+ auto ShardPath = getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
auto Buffer = MemoryBuffer::getFile(ShardPath);
if (!Buffer) {
- elog("Couldn't load {0}: {1}", ShardPath, Buffer.getError().message());
+ elog("Couldn't retrieve {0}: {1}", ShardPath,
+ Buffer.getError().message());
return llvm::make_error<llvm::StringError>(Buffer.getError());
}
+ // FIXME: Change readIndexFile to also look at Hash of the source that
+ // generated index and skip if there is a mismatch.
return readIndexFile(Buffer->get()->getBuffer());
}
- llvm::Error storeShard(llvm::StringRef ShardIdentifier,
- IndexFileOut Shard) const override {
+ bool storeShard(llvm::StringRef ShardIdentifier,
+ IndexFileOut Shard) const override {
auto ShardPath = getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
std::error_code EC;
llvm::raw_fd_ostream OS(ShardPath, EC);
- if (EC)
- return errorCodeToError(EC);
+ if (EC) {
+ elog("Failed to open {0} for writing: {1}", ShardPath, EC.message());
+ return false;
+ }
OS << Shard;
- return llvm::Error::success();
+ return true;
}
- // Sets DiskShardRoot to (Directory + ".clangd-index/") which is the base
- // directory for all shard files.
- DiskBackedIndexStorage(llvm::StringRef Directory) {
- llvm::SmallString<128> CDBDirectory(Directory);
- sys::path::append(CDBDirectory, ".clangd-index/");
- DiskShardRoot = CDBDirectory.str();
+ // Initializes DiskShardRoot to (Directory + ".clangd-index/") which is the
+ // base directory for all shard files. After the initialization succeeds all
+ // subsequent calls are no-op.
+ DiskBackedIndexStorage(llvm::StringRef Directory) : DiskShardRoot(Directory) {
+ sys::path::append(DiskShardRoot, ".clangd-index/");
if (!llvm::sys::fs::exists(DiskShardRoot)) {
std::error_code OK;
std::error_code EC = llvm::sys::fs::create_directory(DiskShardRoot);
@@ -101,7 +106,7 @@ public:
}
};
-std::string getAbsoluteFilePath(const tooling::CompileCommand &Cmd) {
+SmallString<128> getAbsolutePath(const tooling::CompileCommand &Cmd) {
SmallString<128> AbsolutePath;
if (sys::path::is_absolute(Cmd.Filename)) {
AbsolutePath = Cmd.Filename;
@@ -109,7 +114,7 @@ std::string getAbsoluteFilePath(const to
AbsolutePath = Cmd.Directory;
sys::path::append(AbsolutePath, Cmd.Filename);
}
- return AbsolutePath.str();
+ return AbsolutePath;
}
} // namespace
@@ -118,12 +123,10 @@ BackgroundIndex::BackgroundIndex(Context
StringRef ResourceDir,
const FileSystemProvider &FSProvider,
ArrayRef<std::string> URISchemes,
- IndexStorageFactory IndexStorageCreator,
size_t ThreadPoolSize)
: SwapIndex(make_unique<MemIndex>()), ResourceDir(ResourceDir),
FSProvider(FSProvider), BackgroundContext(std::move(BackgroundContext)),
- URISchemes(URISchemes),
- IndexStorageCreator(std::move(IndexStorageCreator)) {
+ URISchemes(URISchemes) {
assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
while (ThreadPoolSize--) {
ThreadPool.emplace_back([this] { run(); });
@@ -182,24 +185,57 @@ void BackgroundIndex::blockUntilIdleForT
void BackgroundIndex::enqueue(StringRef Directory,
tooling::CompileCommand Cmd) {
- BackgroundIndexStorage *IndexStorage = getIndexStorage(Directory);
- if (!IndexStorage)
+ auto IndexStorage = BackgroundIndexStorage::getForDirectory(Directory);
+ if (IndexStorage)
+ loadShard(IndexStorage.get(), Cmd);
+ else
elog("No index storage for: {0}", Directory);
{
std::lock_guard<std::mutex> Lock(QueueMu);
- enqueueLocked(std::move(Cmd), IndexStorage);
+ enqueueLocked(std::move(Cmd), std::move(IndexStorage));
}
QueueCV.notify_all();
}
+void BackgroundIndex::loadShard(BackgroundIndexStorage *IndexStorage,
+ const tooling::CompileCommand &Cmd) {
+ assert(IndexStorage && "No index storage to load from?");
+ auto AbsolutePath = getAbsolutePath(Cmd);
+ auto Shard = IndexStorage->retrieveShard(AbsolutePath);
+ if (Shard) {
+ // FIXME: Updated hashes once we have them in serialized format.
+ // IndexedFileDigests[AbsolutePath] = Hash;
+ IndexedSymbols.update(AbsolutePath,
+ make_unique<SymbolSlab>(std::move(*Shard->Symbols)),
+ make_unique<RefSlab>(std::move(*Shard->Refs)));
+
+ vlog("Loaded {0} from storage", AbsolutePath);
+ }
+}
+
+void BackgroundIndex::loadShards(
+ BackgroundIndexStorage *IndexStorage,
+ const std::vector<tooling::CompileCommand> &Cmds) {
+ assert(IndexStorage && "No index storage to load from?");
+ for (const auto &Cmd : Cmds)
+ loadShard(IndexStorage, Cmd);
+ // FIXME: Maybe we should get rid of this one once we start building index
+ // periodically? Especially if we also offload this task onto the queue.
+ vlog("Rebuilding automatic index");
+ reset(IndexedSymbols.buildIndex(IndexType::Light, DuplicateHandling::Merge,
+ URISchemes));
+}
+
void BackgroundIndex::enqueueAll(StringRef Directory,
const tooling::CompilationDatabase &CDB) {
trace::Span Tracer("BackgroundIndexEnqueueCDB");
// FIXME: this function may be slow. Perhaps enqueue a task to re-read the CDB
// from disk and enqueue the commands asynchronously?
auto Cmds = CDB.getAllCompileCommands();
- BackgroundIndexStorage *IndexStorage = getIndexStorage(Directory);
- if (!IndexStorage)
+ auto IndexStorage = BackgroundIndexStorage::getForDirectory(Directory);
+ if (IndexStorage)
+ loadShards(IndexStorage.get(), Cmds);
+ else
elog("No index storage for: {0}", Directory);
SPAN_ATTACH(Tracer, "commands", int64_t(Cmds.size()));
std::mt19937 Generator(std::random_device{}());
@@ -213,16 +249,18 @@ void BackgroundIndex::enqueueAll(StringR
QueueCV.notify_all();
}
-void BackgroundIndex::enqueueLocked(tooling::CompileCommand Cmd,
- BackgroundIndexStorage *IndexStorage) {
+void BackgroundIndex::enqueueLocked(
+ tooling::CompileCommand Cmd,
+ std::shared_ptr<BackgroundIndexStorage> IndexStorage) {
Queue.push_back(Bind(
- [this, IndexStorage](tooling::CompileCommand Cmd) {
+ [this](tooling::CompileCommand Cmd,
+ std::shared_ptr<BackgroundIndexStorage> IndexStorage) {
std::string Filename = Cmd.Filename;
Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir);
- if (auto Error = index(std::move(Cmd), IndexStorage))
+ if (auto Error = index(std::move(Cmd), IndexStorage.get()))
log("Indexing {0} failed: {1}", Filename, std::move(Error));
},
- std::move(Cmd)));
+ std::move(Cmd), std::move(IndexStorage)));
}
// Resolves URI to file paths with cache.
@@ -318,8 +356,7 @@ void BackgroundIndex::update(StringRef M
IndexFileOut Shard;
Shard.Symbols = SS.get();
Shard.Refs = RS.get();
- if (auto Error = IndexStorage->storeShard(Path, Shard))
- elog("Failed to store shard for {0}: {1}", Path, std::move(Error));
+ IndexStorage->storeShard(Path, Shard);
}
std::lock_guard<std::mutex> Lock(DigestsMu);
@@ -367,7 +404,7 @@ Error BackgroundIndex::index(tooling::Co
BackgroundIndexStorage *IndexStorage) {
trace::Span Tracer("BackgroundIndex");
SPAN_ATTACH(Tracer, "file", Cmd.Filename);
- const std::string AbsolutePath = getAbsoluteFilePath(Cmd);
+ SmallString<128> AbsolutePath = getAbsolutePath(Cmd);
auto FS = FSProvider.getFileSystem();
auto Buf = FS->getBufferForFile(AbsolutePath);
@@ -449,18 +486,8 @@ Error BackgroundIndex::index(tooling::Co
return Error::success();
}
-BackgroundIndexStorage *
-BackgroundIndex::getIndexStorage(llvm::StringRef CDBDirectory) {
- if (!IndexStorageCreator)
- return nullptr;
- auto IndexStorageIt = IndexStorageMap.find(CDBDirectory);
- if (IndexStorageIt == IndexStorageMap.end())
- IndexStorageIt =
- IndexStorageMap
- .insert({CDBDirectory, IndexStorageCreator(CDBDirectory)})
- .first;
- return IndexStorageIt->second.get();
-}
+std::function<std::shared_ptr<BackgroundIndexStorage>(llvm::StringRef)>
+ BackgroundIndexStorage::Factory = nullptr;
} // namespace clangd
} // namespace clang
Modified: clang-tools-extra/trunk/clangd/index/Background.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=346942&r1=346941&r2=346942&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/Background.h (original)
+++ clang-tools-extra/trunk/clangd/index/Background.h Thu Nov 15 02:34:35 2018
@@ -30,30 +30,44 @@ namespace clangd {
// Handles storage and retrieval of index shards.
class BackgroundIndexStorage {
+private:
+ static std::function<std::shared_ptr<BackgroundIndexStorage>(llvm::StringRef)>
+ Factory;
+
public:
+ using FileDigest = decltype(llvm::SHA1::hash({}));
+
// Stores given shard associationg with ShardIdentifier, which can be
// retrieved later on with the same identifier.
- virtual llvm::Error storeShard(llvm::StringRef ShardIdentifier,
- IndexFileOut Shard) const = 0;
+ virtual bool storeShard(llvm::StringRef ShardIdentifier,
+ IndexFileOut Shard) const = 0;
- static std::unique_ptr<BackgroundIndexStorage>
- createDiskStorage(llvm::StringRef CDBDirectory);
+ // Retrieves the shard if found, also returns hash for the source file that
+ // generated this shard.
+ virtual llvm::Expected<IndexFileIn>
+ retrieveShard(llvm::StringRef ShardIdentifier) const = 0;
+
+ template <typename T> static void setStorageFactory(T Factory) {
+ BackgroundIndexStorage::Factory = Factory;
+ }
+
+ static std::shared_ptr<BackgroundIndexStorage>
+ getForDirectory(llvm::StringRef Directory) {
+ if (!Factory)
+ return nullptr;
+ return Factory(Directory);
+ }
};
// Builds an in-memory index by by running the static indexer action over
// all commands in a compilation database. Indexing happens in the background.
-// Takes a factory function to create IndexStorage units for each compilation
-// database. Those databases are identified by directory they are found.
// FIXME: it should also persist its state on disk for fast start.
// FIXME: it should watch for changes to files on disk.
class BackgroundIndex : public SwapIndex {
public:
- using IndexStorageFactory =
- std::function<std::unique_ptr<BackgroundIndexStorage>(llvm::StringRef)>;
// FIXME: resource-dir injection should be hoisted somewhere common.
BackgroundIndex(Context BackgroundContext, llvm::StringRef ResourceDir,
const FileSystemProvider &, ArrayRef<std::string> URISchemes,
- IndexStorageFactory IndexStorageCreator = nullptr,
size_t ThreadPoolSize = llvm::hardware_concurrency());
~BackgroundIndex(); // Blocks while the current task finishes.
@@ -79,6 +93,10 @@ private:
void update(llvm::StringRef MainFile, SymbolSlab Symbols, RefSlab Refs,
const llvm::StringMap<FileDigest> &FilesToUpdate,
BackgroundIndexStorage *IndexStorage);
+ void loadShard(BackgroundIndexStorage *IndexStorage,
+ const tooling::CompileCommand &Cmd);
+ void loadShards(BackgroundIndexStorage *IndexStorage,
+ const std::vector<tooling::CompileCommand> &Cmds);
// configuration
std::string ResourceDir;
@@ -94,17 +112,11 @@ private:
llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.
std::mutex DigestsMu;
- // index storage
- BackgroundIndexStorage *getIndexStorage(llvm::StringRef CDBDirectory);
- // Maps CDB Directory to index storage.
- llvm::StringMap<std::unique_ptr<BackgroundIndexStorage>> IndexStorageMap;
- IndexStorageFactory IndexStorageCreator;
-
// queue management
using Task = std::function<void()>;
void run(); // Main loop executed by Thread. Runs tasks from Queue.
void enqueueLocked(tooling::CompileCommand Cmd,
- BackgroundIndexStorage *IndexStorage);
+ std::shared_ptr<BackgroundIndexStorage> IndexStorage);
std::mutex QueueMu;
unsigned NumActiveTasks = 0; // Only idle when queue is empty *and* no tasks.
std::condition_variable QueueCV;
Modified: clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp?rev=346942&r1=346941&r2=346942&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp Thu Nov 15 02:34:35 2018
@@ -78,23 +78,38 @@ TEST(BackgroundIndexTest, IndexTwoFiles)
FileURI("unittest:///root/B.cc")}));
}
-TEST(BackgroundIndexTest, ShardStorageWriteTest) {
+TEST(BackgroundIndexTest, ShardStorageTest) {
class MemoryShardStorage : public BackgroundIndexStorage {
mutable std::mutex StorageMu;
llvm::StringMap<std::string> &Storage;
+ size_t &CacheHits;
public:
- MemoryShardStorage(llvm::StringMap<std::string> &Storage)
- : Storage(Storage) {}
- llvm::Error storeShard(llvm::StringRef ShardIdentifier,
- IndexFileOut Shard) const override {
+ MemoryShardStorage(llvm::StringMap<std::string> &Storage, size_t &CacheHits)
+ : Storage(Storage), CacheHits(CacheHits) {}
+
+ bool storeShard(llvm::StringRef ShardIdentifier,
+ IndexFileOut Shard) const override {
std::lock_guard<std::mutex> Lock(StorageMu);
std::string &str = Storage[ShardIdentifier];
llvm::raw_string_ostream OS(str);
OS << Shard;
OS.flush();
- return llvm::Error::success();
+ return true;
+ }
+ llvm::Expected<IndexFileIn>
+ retrieveShard(llvm::StringRef ShardIdentifier) const override {
+ std::lock_guard<std::mutex> Lock(StorageMu);
+ if (Storage.find(ShardIdentifier) == Storage.end())
+ return llvm::make_error<llvm::StringError>(
+ "Shard not found.", llvm::inconvertibleErrorCode());
+ auto IndexFile = readIndexFile(Storage[ShardIdentifier]);
+ if (!IndexFile)
+ return IndexFile;
+ CacheHits++;
+ return IndexFile;
}
+ bool initialize(llvm::StringRef Directory) { return true; }
};
MockFSProvider FS;
FS.Files[testPath("root/A.h")] = R"cpp(
@@ -106,10 +121,11 @@ TEST(BackgroundIndexTest, ShardStorageWr
"#include \"A.h\"\nvoid g() { (void)common; }";
llvm::StringMap<std::string> Storage;
- BackgroundIndex::IndexStorageFactory MemoryStorageFactory =
- [&Storage](llvm::StringRef) {
- return llvm::make_unique<MemoryShardStorage>(Storage);
- };
+ size_t CacheHits = 0;
+ BackgroundIndexStorage::setStorageFactory(
+ [&Storage, &CacheHits](llvm::StringRef) {
+ return std::make_shared<MemoryShardStorage>(Storage, CacheHits);
+ });
tooling::CompileCommand Cmd;
Cmd.Filename = testPath("root/A.cc");
@@ -117,11 +133,22 @@ TEST(BackgroundIndexTest, ShardStorageWr
Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
// Check nothing is loaded from Storage, but A.cc and A.h has been stored.
{
- BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"},
- MemoryStorageFactory);
+ BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"});
+ Idx.enqueue(testPath("root"), Cmd);
+ Idx.blockUntilIdleForTest();
+ }
+ EXPECT_EQ(CacheHits, 0U);
+ EXPECT_EQ(Storage.size(), 2U);
+ EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
+ EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
+
+ // Check A.cc has been loaded from cache.
+ {
+ BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"});
Idx.enqueue(testPath("root"), Cmd);
Idx.blockUntilIdleForTest();
}
+ EXPECT_EQ(CacheHits, 1U);
EXPECT_EQ(Storage.size(), 2U);
EXPECT_NE(Storage.find(testPath("root/A.h")), Storage.end());
EXPECT_NE(Storage.find(testPath("root/A.cc")), Storage.end());
More information about the cfe-commits
mailing list