This revision was automatically updated to reflect the committed changes.
sammccall marked 4 inline comments as done.
Closed by commit rGff616f74c3b4: [clangd] Cache config files for 5 seconds,
without revalidating with stat. (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D83755?vs=277723&id=277836#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83755/new/
https://reviews.llvm.org/D83755
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ConfigProvider.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
+++ 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 <atomic>
+#include <chrono>
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
Index: clang-tools-extra/clangd/ConfigProvider.h
===================================================================
--- clang-tools-extra/clangd/ConfigProvider.h
+++ 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 <chrono>
#include <string>
#include <vector>
@@ -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<std::chrono::steady_clock::time_point> FreshTime;
};
/// Used to report problems in parsing or interpreting a config.
Index: clang-tools-extra/clangd/ConfigProvider.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigProvider.cpp
+++ 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 <chrono>
#include <mutex>
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<CompiledFragment, 1> 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<std::chrono::steady_clock::time_point> FreshTime,
std::vector<CompiledFragment> &Out) {
+ std::lock_guard<std::mutex> 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<std::mutex> 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<CompiledFragment>
getFragments(const Params &P, DiagnosticCallback DC) const override {
std::vector<CompiledFragment> 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<CompiledFragment> Result;
for (FileConfigCache *Cache : Caches)
- Cache->read(FS, DC, Result);
+ Cache->read(FS, DC, P.FreshTime, Result);
return Result;
};
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -48,6 +48,7 @@
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/raw_ostream.h"
#include <algorithm>
+#include <chrono>
#include <future>
#include <memory>
#include <mutex>
@@ -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));
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
