diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -48,6 +48,7 @@ #include "llvm/Support/ScopedPrinter.h" #include "llvm/Support/raw_ostream.h" #include +#include #include #include #include @@ -762,6 +763,9 @@ return Context::current().clone(); config::Params Params; + // Don't reread config files excessively often. + // FIXME: when we see a config file change event, use the event timestamp. + Params.FreshTime = std::chrono::steady_clock::now() - std::chrono::seconds(5); llvm::SmallString<256> PosixPath; if (!File.empty()) { assert(llvm::sys::path::is_absolute(File)); diff --git a/clang-tools-extra/clangd/ConfigProvider.h b/clang-tools-extra/clangd/ConfigProvider.h --- a/clang-tools-extra/clangd/ConfigProvider.h +++ b/clang-tools-extra/clangd/ConfigProvider.h @@ -20,6 +20,7 @@ #include "llvm/ADT/FunctionExtras.h" #include "llvm/Support/SMLoc.h" #include "llvm/Support/SourceMgr.h" +#include #include #include @@ -34,6 +35,10 @@ /// Absolute path to a source file we're applying the config to. Unix slashes. /// Empty if not configuring a particular file. llvm::StringRef Path; + /// Hint that stale data is OK to improve performance (e.g. avoid IO). + /// FreshTime sets a bound for how old the data can be. + /// If not set, providers should validate caches against the data source. + llvm::Optional FreshTime; }; /// Used to report problems in parsing or interpreting a config. diff --git a/clang-tools-extra/clangd/ConfigProvider.cpp b/clang-tools-extra/clangd/ConfigProvider.cpp --- a/clang-tools-extra/clangd/ConfigProvider.cpp +++ b/clang-tools-extra/clangd/ConfigProvider.cpp @@ -11,8 +11,10 @@ #include "ConfigFragment.h" #include "support/ThreadsafeFS.h" #include "support/Trace.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/StringMap.h" #include "llvm/Support/Path.h" +#include #include namespace clang { @@ -22,21 +24,18 @@ // Threadsafe cache around reading a YAML config file from disk. class FileConfigCache { std::mutex Mu; + std::chrono::steady_clock::time_point ValidTime = {}; llvm::SmallVector CachedValue; llvm::sys::TimePoint<> MTime = {}; unsigned Size = -1; - void updateCacheLocked(const llvm::vfs::Status &Stat, - llvm::vfs::FileSystem &FS, DiagnosticCallback DC) { - if (Size == Stat.getSize() && MTime == Stat.getLastModificationTime()) - return; // Already valid. - - Size = Stat.getSize(); - MTime = Stat.getLastModificationTime(); + // Called once we are sure we want to read the file. + // REQUIRES: Cache keys are set. Mutex must be held. + void fillCacheFromDisk(llvm::vfs::FileSystem &FS, DiagnosticCallback DC) { CachedValue.clear(); auto Buf = FS.getBufferForFile(Path); - // If stat() succeeds but we failed to read, don't cache failure. + // If we failed to read (but stat succeeded), don't cache failure. if (!Buf) { Size = -1; MTime = {}; @@ -68,19 +67,40 @@ // - allow caches to be reused based on short elapsed walltime // - allow latency-sensitive operations to skip revalidating the cache void read(const ThreadsafeFS &TFS, DiagnosticCallback DC, + llvm::Optional FreshTime, std::vector &Out) { + std::lock_guard Lock(Mu); + // We're going to update the cache and return whatever's in it. + auto Return = llvm::make_scope_exit( + [&] { llvm::copy(CachedValue, std::back_inserter(Out)); }); + + // Return any sufficiently recent result without doing any further work. + if (FreshTime && ValidTime >= FreshTime) + return; + + // Ensure we bump the ValidTime at the end to allow for reuse. + auto MarkTime = llvm::make_scope_exit( + [&] { ValidTime = std::chrono::steady_clock::now(); }); + + // Stat is cheaper than opening the file, it's usually unchanged. assert(llvm::sys::path::is_absolute(Path)); auto FS = TFS.view(/*CWD=*/llvm::None); auto Stat = FS->status(Path); + // If there's no file, the result is empty. Ensure we have an invalid key. if (!Stat || !Stat->isRegularFile()) { - // No point taking the lock to clear the cache. We know what to return. - // If the file comes back we'll invalidate the cache at that point. + MTime = {}; + Size = -1; + CachedValue.clear(); return; } + // If the modified-time and size match, assume the content does too. + if (Size == Stat->getSize() && MTime == Stat->getLastModificationTime()) + return; - std::lock_guard Lock(Mu); - updateCacheLocked(*Stat, *FS, DC); - llvm::copy(CachedValue, std::back_inserter(Out)); + // OK, the file has actually changed. Update cache key, compute new value. + Size = Stat->getSize(); + MTime = Stat->getLastModificationTime(); + fillCacheFromDisk(*FS, DC); } }; @@ -93,7 +113,7 @@ std::vector getFragments(const Params &P, DiagnosticCallback DC) const override { std::vector Result; - Cache.read(FS, DC, Result); + Cache.read(FS, DC, P.FreshTime, Result); return Result; }; @@ -158,7 +178,7 @@ // This will take a (per-file) lock for each file that actually exists. std::vector Result; for (FileConfigCache *Cache : Caches) - Cache->read(FS, DC, Result); + Cache->read(FS, DC, P.FreshTime, Result); return Result; }; diff --git a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp --- a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp @@ -10,10 +10,11 @@ #include "ConfigProvider.h" #include "ConfigTesting.h" #include "TestFS.h" +#include "llvm/Support/SourceMgr.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -#include "llvm/Support/SourceMgr.h" #include +#include namespace clang { namespace clangd { @@ -150,6 +151,43 @@ EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz")); } +TEST(ProviderTest, Staleness) { + MockFS FS; + + auto StartTime = std::chrono::steady_clock::now(); + Params StaleOK; + StaleOK.FreshTime = StartTime; + Params MustBeFresh; + MustBeFresh.FreshTime = StartTime + std::chrono::hours(1); + CapturedDiags Diags; + auto P = Provider::fromYAMLFile(testPath("foo.yaml"), FS); + + // Initial query always reads, regardless of policy. + FS.Files["foo.yaml"] = AddFooWithErr; + auto Cfg = P->getConfig(StaleOK, Diags.callback()); + EXPECT_THAT(Diags.Diagnostics, + ElementsAre(DiagMessage("Unknown CompileFlags key Unknown"))); + EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo")); + Diags.Diagnostics.clear(); + + // Stale value reused by policy. + FS.Files["foo.yaml"] = AddBarBaz; + Cfg = P->getConfig(StaleOK, Diags.callback()); + EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached, not re-parsed"; + EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo")); + + // Cache revalidated by policy. + Cfg = P->getConfig(MustBeFresh, Diags.callback()); + EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors"; + EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz")); + + // Cache revalidated by (default) policy. + FS.Files.erase("foo.yaml"); + Cfg = P->getConfig(Params(), Diags.callback()); + EXPECT_THAT(Diags.Diagnostics, IsEmpty()); + EXPECT_THAT(getAddedArgs(Cfg), IsEmpty()); +} + } // namespace } // namespace config } // namespace clangd