Commit ff616f74 authored by Sam McCall's avatar Sam McCall
Browse files

[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
parent e6c01642

clang-tools-extra/clangd/ClangdServer.cpp

+4 −0
Original line number Diff line number Diff line
@@ -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));

clang-tools-extra/clangd/ConfigProvider.cpp

+35 −15
Original line number Diff line number Diff line
@@ -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 @@ public: 
  // - 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;

    };

clang-tools-extra/clangd/ConfigProvider.h

+5 −0
Original line number Diff line number Diff line
@@ -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.

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

+39 −1
Original line number Diff line number Diff line
@@ -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