[clangd] Cache config files for 5 seconds, without revalidating with stat.

[sammccall]

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.

[kadircet]

thanks, LGTM

clang-tools-extra/clangd/ConfigProvider.cpp

24

looks like an unwated artifact

103

nit: the rest of this function is also updating the cache now, maybe inline the function call?

clang-tools-extra/clangd/ConfigProvider.h

41

i would've suggested storing a duration here instead of time point, but as discussed offline this is only done once in the clangdserver and storing a time point might help with invalidating the cache based on filewatcher events easily.

clang-tools-extra/clangd/ClangdServer.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
+#include <chrono>
 #include <future>
 #include <memory>
 #include <mutex>
 #include <type_traits>
 
 namespace clang {
 namespace clangd {
 namespace {
llvm::StringMap<TUScheduler::FileStats> ClangdServer::fileStats() const {
   return WorkScheduler.fileStats();
 }
 
 Context ClangdServer::createProcessingContext(PathRef File) const {
   if (!ConfigProvider)
     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));
     llvm::sys::path::native(File, PosixPath, llvm::sys::path::Style::posix);
     Params.Path = PosixPath.str();
   }
 
   auto DiagnosticHandler = [](const llvm::SMDiagnostic &Diag) {

clang-tools-extra/clangd/ConfigProvider.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGPROVIDER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIGPROVIDER_H
 
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SourceMgr.h"
+#include <chrono>
 #include <string>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 struct Config;
 class ThreadsafeFS;
 namespace config {
 
 /// Describes the context used to evaluate configuration fragments.
 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.
 /// Errors reflect structurally invalid config that should be user-visible.
 /// Warnings reflect e.g. unknown properties that are recoverable.
 using DiagnosticCallback = llvm::function_ref<void(const llvm::SMDiagnostic &)>;
 
 /// A chunk of configuration that has been fully analyzed and is ready to apply.

clang-tools-extra/clangd/ConfigProvider.cpp

 //===--- ConfigProvider.cpp - Loading of user configuration ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ConfigProvider.h"
 #include "Config.h"
 #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();
+  // 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 = {};
       return;
     }
 
     // If file changed between stat and open, we don't know its mtime.
     // For simplicity, don't cache the value in this case (use a bad key).
public:
 
   // Retrieves up-to-date config fragments from disk.
   // A cached result may be reused if the mtime and size are unchanged.
   // (But several concurrent read()s can miss the cache after a single change).
   // Future performance ideas:
   // - 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);
   }
 };
 
 std::unique_ptr<Provider> Provider::fromYAMLFile(llvm::StringRef AbsPath,
                                                  const ThreadsafeFS &FS) {
   class AbsFileProvider : public Provider {
     mutable FileConfigCache Cache; // threadsafe
     const ThreadsafeFS &FS;
 
     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;
     };
 
   public:
     AbsFileProvider(llvm::StringRef Path, const ThreadsafeFS &FS) : FS(FS) {
       assert(llvm::sys::path::is_absolute(Path));
       Cache.Path = Path.str();
     }
getFragments(const Params &P, DiagnosticCallback DC) const override {
           }
           Caches.push_back(&R.first->second);
         }
       }
       // Finally query each individual file.
       // 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;
     };
 
   public:
     RelFileProvider(llvm::StringRef RelPath, const ThreadsafeFS &FS)
         : RelPath(RelPath), FS(FS) {
       assert(llvm::sys::path::is_relative(RelPath));
     }

clang-tools-extra/clangd/unittests/ConfigProviderTests.cpp

 //===-- ConfigProviderTests.cpp -------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Config.h"
 #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 {
 namespace config {
 namespace {
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
 
TEST(ProviderTest, FromAncestorRelativeYAMLFiles) {
   EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
 
   FS.Files.erase("a/foo.yaml");
   Cfg = P->getConfig(ABCParams, Diags.callback());
   EXPECT_THAT(Diags.Diagnostics, IsEmpty());
   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
 } // namespace clang