Author: kadircet Date: Thu Nov 15 02:34:39 2018 New Revision: 346943 URL: http://llvm.org/viewvc/llvm-project?rev=346943&view=rev Log: Revert "Address comments"
This reverts commit 19a39b14eab2b5339325e276262b177357d6b412. 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=346943&r1=346942&r2=346943&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Background.cpp (original) +++ clang-tools-extra/trunk/clangd/index/Background.cpp Thu Nov 15 02:34:39 2018 @@ -56,77 +56,15 @@ getShardPathFromFilePath(llvm::SmallStri return ShardRoot; } -// Uses disk as a storage for index shards. Creates a directory called -// ".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 { - llvm::SmallString<128> DiskShardRoot; - -public: - llvm::Expected<IndexFileIn> - retrieveShard(llvm::StringRef ShardIdentifier) const override { - auto ShardPath = getShardPathFromFilePath(DiskShardRoot, ShardIdentifier); - auto Buffer = MemoryBuffer::getFile(ShardPath); - if (!Buffer) { - 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()); - } - - 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) { - elog("Failed to open {0} for writing: {1}", ShardPath, EC.message()); - return false; - } - OS << Shard; - return true; - } - - // 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); - if (EC != OK) { - elog("Failed to create {0}: {1}", DiskShardRoot, EC.message()); - } - } - } -}; - -SmallString<128> getAbsolutePath(const tooling::CompileCommand &Cmd) { - SmallString<128> AbsolutePath; - if (sys::path::is_absolute(Cmd.Filename)) { - AbsolutePath = Cmd.Filename; - } else { - AbsolutePath = Cmd.Directory; - sys::path::append(AbsolutePath, Cmd.Filename); - } - return AbsolutePath; -} - } // namespace -BackgroundIndex::BackgroundIndex(Context BackgroundContext, - StringRef ResourceDir, - const FileSystemProvider &FSProvider, - ArrayRef<std::string> URISchemes, - size_t ThreadPoolSize) +BackgroundIndex::BackgroundIndex( + Context BackgroundContext, StringRef ResourceDir, + const FileSystemProvider &FSProvider, ArrayRef<std::string> URISchemes, + std::unique_ptr<ShardStorage> IndexShardStorage, size_t ThreadPoolSize) : SwapIndex(make_unique<MemIndex>()), ResourceDir(ResourceDir), FSProvider(FSProvider), BackgroundContext(std::move(BackgroundContext)), - URISchemes(URISchemes) { + URISchemes(URISchemes), IndexShardStorage(std::move(IndexShardStorage)) { assert(ThreadPoolSize > 0 && "Thread pool size can't be zero."); while (ThreadPoolSize--) { ThreadPool.emplace_back([this] { run(); }); @@ -185,58 +123,19 @@ void BackgroundIndex::blockUntilIdleForT void BackgroundIndex::enqueue(StringRef Directory, tooling::CompileCommand Cmd) { - 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), std::move(IndexStorage)); + enqueueLocked(std::move(Cmd)); } 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(); - 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{}()); std::shuffle(Cmds.begin(), Cmds.end(), Generator); @@ -244,23 +143,26 @@ void BackgroundIndex::enqueueAll(StringR { std::lock_guard<std::mutex> Lock(QueueMu); for (auto &Cmd : Cmds) - enqueueLocked(std::move(Cmd), IndexStorage); + enqueueLocked(std::move(Cmd)); } QueueCV.notify_all(); } -void BackgroundIndex::enqueueLocked( - tooling::CompileCommand Cmd, - std::shared_ptr<BackgroundIndexStorage> IndexStorage) { +void BackgroundIndex::enqueueLocked(tooling::CompileCommand Cmd) { + // Initialize storage to project root. Since Initialize is no-op for multiple + // calls we can simply call it for each file. + if (IndexShardStorage && !IndexShardStorage->initialize(Cmd.Directory)) { + elog("Failed to initialize shard storage"); + IndexShardStorage.reset(); + } Queue.push_back(Bind( - [this](tooling::CompileCommand Cmd, - std::shared_ptr<BackgroundIndexStorage> IndexStorage) { + [this](tooling::CompileCommand Cmd) { std::string Filename = Cmd.Filename; Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir); - if (auto Error = index(std::move(Cmd), IndexStorage.get())) + if (auto Error = index(std::move(Cmd))) log("Indexing {0} failed: {1}", Filename, std::move(Error)); }, - std::move(Cmd), std::move(IndexStorage))); + std::move(Cmd))); } // Resolves URI to file paths with cache. @@ -296,8 +198,7 @@ private: /// Given index results from a TU, only update files in \p FilesToUpdate. void BackgroundIndex::update(StringRef MainFile, SymbolSlab Symbols, RefSlab Refs, - const StringMap<FileDigest> &FilesToUpdate, - BackgroundIndexStorage *IndexStorage) { + const StringMap<FileDigest> &FilesToUpdate) { // Partition symbols/references into files. struct File { DenseSet<const Symbol *> Symbols; @@ -349,14 +250,13 @@ void BackgroundIndex::update(StringRef M auto RS = llvm::make_unique<RefSlab>(std::move(Refs).build()); auto Hash = FilesToUpdate.lookup(Path); - // We need to store shards before updating the index, since the latter - // consumes slabs. + // Put shards into storage for subsequent use. // FIXME: Store Hash in the Shard. - if (IndexStorage) { + if (IndexShardStorage) { IndexFileOut Shard; Shard.Symbols = SS.get(); Shard.Refs = RS.get(); - IndexStorage->storeShard(Path, Shard); + IndexShardStorage->storeShard(Path, Shard); } std::lock_guard<std::mutex> Lock(DigestsMu); @@ -370,8 +270,7 @@ void BackgroundIndex::update(StringRef M // Creates a filter to not collect index results from files with unchanged // digests. -// \p FileDigests contains file digests for the current indexed files, and all -// changed files will be added to \p FilesToUpdate. +// \p FileDigests contains file digests for the current indexed files, and all changed files will be added to \p FilesToUpdate. decltype(SymbolCollector::Options::FileFilter) createFileFilter( const llvm::StringMap<BackgroundIndex::FileDigest> &FileDigests, llvm::StringMap<BackgroundIndex::FileDigest> &FilesToUpdate) { @@ -400,11 +299,16 @@ decltype(SymbolCollector::Options::FileF }; } -Error BackgroundIndex::index(tooling::CompileCommand Cmd, - BackgroundIndexStorage *IndexStorage) { +Error BackgroundIndex::index(tooling::CompileCommand Cmd) { trace::Span Tracer("BackgroundIndex"); SPAN_ATTACH(Tracer, "file", Cmd.Filename); - SmallString<128> AbsolutePath = getAbsolutePath(Cmd); + SmallString<128> AbsolutePath; + if (sys::path::is_absolute(Cmd.Filename)) { + AbsolutePath = Cmd.Filename; + } else { + AbsolutePath = Cmd.Directory; + sys::path::append(AbsolutePath, Cmd.Filename); + } auto FS = FSProvider.getFileSystem(); auto Buf = FS->getBufferForFile(AbsolutePath); @@ -419,6 +323,18 @@ Error BackgroundIndex::index(tooling::Co if (IndexedFileDigests.lookup(AbsolutePath) == Hash) { vlog("No need to index {0}, already up to date", AbsolutePath); return Error::success(); + } else if (IndexShardStorage) { // Check if shard storage has the index. + auto Shard = IndexShardStorage->retrieveShard(AbsolutePath, Hash); + if (Shard) { + // FIXME: We might still want to re-index headers. + 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); + return Error::success(); + } } DigestsSnapshot = IndexedFileDigests; @@ -468,8 +384,7 @@ Error BackgroundIndex::index(tooling::Co Symbols.size(), Refs.numRefs()); SPAN_ATTACH(Tracer, "symbols", int(Symbols.size())); SPAN_ATTACH(Tracer, "refs", int(Refs.numRefs())); - update(AbsolutePath, std::move(Symbols), std::move(Refs), FilesToUpdate, - IndexStorage); + update(AbsolutePath, std::move(Symbols), std::move(Refs), FilesToUpdate); { // Make sure hash for the main file is always updated even if there is no // index data in it. @@ -486,8 +401,59 @@ Error BackgroundIndex::index(tooling::Co return Error::success(); } -std::function<std::shared_ptr<BackgroundIndexStorage>(llvm::StringRef)> - BackgroundIndexStorage::Factory = nullptr; +llvm::Expected<IndexFileIn> +DiskShardStorage::retrieveShard(llvm::StringRef ShardIdentifier, + FileDigest Hash) const { + assert(Initialized && "Not initialized?"); + llvm::SmallString<128> ShardPath; + { + std::lock_guard<std::mutex> Lock(DiskShardRootMu); + ShardPath = getShardPathFromFilePath(DiskShardRoot, ShardIdentifier); + } + auto Buffer = MemoryBuffer::getFile(ShardPath); + if (!Buffer) { + 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()); +} + +bool DiskShardStorage::storeShard(llvm::StringRef ShardIdentifier, + IndexFileOut Shard) const { + assert(Initialized && "Not initialized?"); + llvm::SmallString<128> ShardPath; + { + std::lock_guard<std::mutex> Lock(DiskShardRootMu); + ShardPath = getShardPathFromFilePath(DiskShardRoot, ShardIdentifier); + } + std::error_code EC; + llvm::raw_fd_ostream OS(ShardPath, EC); + if (EC) { + elog("Failed to open {0} for writing: {1}", ShardPath, EC.message()); + return false; + } + OS << Shard; + return true; +} + +bool DiskShardStorage::initialize(llvm::StringRef Directory) { + if (Initialized) + return true; + std::lock_guard<std::mutex> Lock(DiskShardRootMu); + 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); + if (EC != OK) { + elog("Failed to create {0}: {1}", DiskShardRoot, EC.message()); + return Initialized = false; + } + } + return Initialized = true; +} } // 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=346943&r1=346942&r2=346943&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Background.h (original) +++ clang-tools-extra/trunk/clangd/index/Background.h Thu Nov 15 02:34:39 2018 @@ -28,35 +28,15 @@ namespace clang { namespace clangd { -// Handles storage and retrieval of index shards. -class BackgroundIndexStorage { -private: - static std::function<std::shared_ptr<BackgroundIndexStorage>(llvm::StringRef)> - Factory; - +// Base class for Shard Storage operations. See DiskShardStorage for more info. +class ShardStorage { public: using FileDigest = decltype(llvm::SHA1::hash({})); - - // Stores given shard associationg with ShardIdentifier, which can be - // retrieved later on with the same identifier. virtual bool storeShard(llvm::StringRef ShardIdentifier, IndexFileOut Shard) const = 0; - - // 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); - } + retrieveShard(llvm::StringRef ShardIdentifier, FileDigest Hash) const = 0; + virtual bool initialize(llvm::StringRef Directory) = 0; }; // Builds an in-memory index by by running the static indexer action over @@ -68,6 +48,7 @@ public: // FIXME: resource-dir injection should be hoisted somewhere common. BackgroundIndex(Context BackgroundContext, llvm::StringRef ResourceDir, const FileSystemProvider &, ArrayRef<std::string> URISchemes, + std::unique_ptr<ShardStorage> IndexShardStorage = nullptr, size_t ThreadPoolSize = llvm::hardware_concurrency()); ~BackgroundIndex(); // Blocks while the current task finishes. @@ -91,22 +72,17 @@ public: private: /// Given index results from a TU, only update files in \p FilesToUpdate. 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); + const llvm::StringMap<FileDigest> &FilesToUpdate); // configuration std::string ResourceDir; const FileSystemProvider &FSProvider; Context BackgroundContext; std::vector<std::string> URISchemes; + std::unique_ptr<ShardStorage> IndexShardStorage; // index state - llvm::Error index(tooling::CompileCommand, - BackgroundIndexStorage *IndexStorage); + llvm::Error index(tooling::CompileCommand); FileSymbols IndexedSymbols; llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path. @@ -115,8 +91,7 @@ private: // queue management using Task = std::function<void()>; void run(); // Main loop executed by Thread. Runs tasks from Queue. - void enqueueLocked(tooling::CompileCommand Cmd, - std::shared_ptr<BackgroundIndexStorage> IndexStorage); + void enqueueLocked(tooling::CompileCommand Cmd); std::mutex QueueMu; unsigned NumActiveTasks = 0; // Only idle when queue is empty *and* no tasks. std::condition_variable QueueCV; @@ -125,6 +100,30 @@ private: std::vector<std::thread> ThreadPool; // FIXME: Abstract this away. }; +// Handles storage and retrieval of index shards into disk. Requires Initialize +// to be called before storing or retrieval. Creates a directory called +// ".clangd-index/" under the path provided during initialize. This class is +// thread-safe. +class DiskShardStorage : public ShardStorage { + mutable std::mutex DiskShardRootMu; + llvm::SmallString<128> DiskShardRoot; + bool Initialized; + +public: + // Retrieves the shard if found and contents are consistent with the provided + // Hash. + llvm::Expected<IndexFileIn> retrieveShard(llvm::StringRef ShardIdentifier, + FileDigest Hash) const; + + // Stores given shard with name ShardIdentifier under initialized directory. + bool storeShard(llvm::StringRef ShardIdentifier, IndexFileOut Shard) const; + + // Initializes DiskShardRoot to (Directory + ".clangd-index/") which is the + // base directory for all shard files. After the initialization succeeds all + // subsequent calls or no-op. + bool initialize(llvm::StringRef Directory); +}; + } // namespace clangd } // namespace clang 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=346943&r1=346942&r2=346943&view=diff ============================================================================== --- clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp (original) +++ clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp Thu Nov 15 02:34:39 2018 @@ -79,7 +79,7 @@ TEST(BackgroundIndexTest, IndexTwoFiles) } TEST(BackgroundIndexTest, ShardStorageTest) { - class MemoryShardStorage : public BackgroundIndexStorage { + class MemoryShardStorage : public ShardStorage { mutable std::mutex StorageMu; llvm::StringMap<std::string> &Storage; size_t &CacheHits; @@ -88,8 +88,7 @@ TEST(BackgroundIndexTest, ShardStorageTe MemoryShardStorage(llvm::StringMap<std::string> &Storage, size_t &CacheHits) : Storage(Storage), CacheHits(CacheHits) {} - bool storeShard(llvm::StringRef ShardIdentifier, - IndexFileOut Shard) const override { + bool storeShard(llvm::StringRef ShardIdentifier, IndexFileOut Shard) const { std::lock_guard<std::mutex> Lock(StorageMu); std::string &str = Storage[ShardIdentifier]; llvm::raw_string_ostream OS(str); @@ -97,8 +96,8 @@ TEST(BackgroundIndexTest, ShardStorageTe OS.flush(); return true; } - llvm::Expected<IndexFileIn> - retrieveShard(llvm::StringRef ShardIdentifier) const override { + llvm::Expected<IndexFileIn> retrieveShard(llvm::StringRef ShardIdentifier, + FileDigest Hash) const { std::lock_guard<std::mutex> Lock(StorageMu); if (Storage.find(ShardIdentifier) == Storage.end()) return llvm::make_error<llvm::StringError>( @@ -119,21 +118,17 @@ TEST(BackgroundIndexTest, ShardStorageTe )cpp"; FS.Files[testPath("root/A.cc")] = "#include \"A.h\"\nvoid g() { (void)common; }"; - llvm::StringMap<std::string> 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"); Cmd.Directory = testPath("root"); 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"}); + BackgroundIndex Idx( + Context::empty(), "", FS, /*URISchemes=*/{"unittest"}, + /*IndexShardStorage=*/ + llvm::make_unique<MemoryShardStorage>(Storage, CacheHits)); Idx.enqueue(testPath("root"), Cmd); Idx.blockUntilIdleForTest(); } @@ -142,9 +137,11 @@ TEST(BackgroundIndexTest, ShardStorageTe 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"}); + BackgroundIndex Idx( + Context::empty(), "", FS, /*URISchemes=*/{"unittest"}, + /*IndexShardStorage=*/ + llvm::make_unique<MemoryShardStorage>(Storage, CacheHits)); Idx.enqueue(testPath("root"), Cmd); Idx.blockUntilIdleForTest(); } @@ -152,6 +149,7 @@ TEST(BackgroundIndexTest, ShardStorageTe 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()); + // B_CC is dropped as we don't collect symbols from A.h in this compilation. } } // namespace clangd _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits