sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
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,32 +11,31 @@
#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 {
namespace clangd {
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();
+ void updateCacheLocked(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();
+ updateCacheLocked(*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>
@@ -750,6 +751,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
