[clang-tools-extra] ff616f7 - [clangd] Cache config files for 5 seconds, without revalidating with stat.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 14 08:02:25 PDT 2020
Author: Sam McCall
Date: 2020-07-14T17:00:41+02:00
New Revision: ff616f74c3b45e0890b53d92fcfc6a9d18f4bfdd
URL: https://github.com/llvm/llvm-project/commit/ff616f74c3b45e0890b53d92fcfc6a9d18f4bfdd
DIFF: https://github.com/llvm/llvm-project/commit/ff616f74c3b45e0890b53d92fcfc6a9d18f4bfdd.diff
LOG: [clangd] Cache config files for 5 seconds, without revalidating with stat.
Summary:
This is motivated by:
- code completion: nice to do no i/o on the request path
- background index: deciding whether to enqueue each file would stat the config
file thousands of times in quick succession.
Currently it's applied uniformly to all requests though.
This gives up on performing stat() outside the lock, all this achieves is
letting multiple threads stat concurrently (and thus finish without contention
for nonexistent files).
The ability to finish without IO (just mutex lock + integer check) should
outweigh this, and is less sensitive to platform IO characteristics.
Reviewers: kadircet
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D83755
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index c33cdffcb0ca..ec4855659501 100644
--- 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 <algorithm>
+#include <chrono>
#include <future>
#include <memory>
#include <mutex>
@@ -762,6 +763,9 @@ Context ClangdServer::createProcessingContext(PathRef File) const {
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.cpp b/clang-tools-extra/clangd/ConfigProvider.cpp
index 4b466d53e293..1f0f727998e3 100644
--- 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 <chrono>
#include <mutex>
namespace clang {
@@ -22,21 +24,18 @@ namespace config {
// 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 @@ class FileConfigCache {
// - 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::unique_ptr<Provider> Provider::fromYAMLFile(llvm::StringRef AbsPath,
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 @@ Provider::fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath,
// 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;
};
diff --git a/clang-tools-extra/clangd/ConfigProvider.h b/clang-tools-extra/clangd/ConfigProvider.h
index a773e56b3bd7..f6c26bde9e0f 100644
--- 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 <chrono>
#include <string>
#include <vector>
@@ -34,6 +35,10 @@ struct Params {
/// 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.
diff --git a/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp b/clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp
index 122b55cf64e0..ff3198e8d335 100644
--- 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 <atomic>
+#include <chrono>
namespace clang {
namespace clangd {
@@ -150,6 +151,43 @@ TEST(ProviderTest, FromAncestorRelativeYAMLFiles) {
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
More information about the cfe-commits
mailing list