[clangd] Config: loading and caching config from disk.

[sammccall]

Summary

The Provider extension point is designed to also be implemented by
ClangdLSPServer (to inject config-over-lsp) and likely by embedders.

[njames93]

clang-tools-extra/clangd/ConfigProvider.h

59

a fixed path

[kadircet]

thanks looks really cool!

clang-tools-extra/clangd/ConfigProvider.cpp

40

I believe this function is going to be hot, I would suggest putting a span here to track the latencies

42

i suppose doing this IO on most of our features are OK, as they tend to search for .clang-format at least (even the ancestor traversal), but I wonder whether these will also be triggered for code completion and signature help, as they currently don't do any IO (apart from preamble deserialization, and maybe one patched include file).

Not a concern for this patch, but something to keep in mind when integrating with the rest.

58

is this really necessary ? we are setting the cache key to old stat values anyway, so the next read shouldn't match neither the Size nor MTime.

68

nit: I've found the nestedness a bit overwhelming, wdyt about putting this into a scope_exit function and using early returns above ?

121

ummm, what ? is this a bug in path iterator that should be fixed rather than worked around?

182

nit: I think this looks a bit ... surprising

clang-tools-extra/clangd/ConfigProvider.h

72

maybe also mention precedence order ? I suppose it already has the natural, the latter one will override the former behavior, but saying out explicitly wouldn't hurt.

74

maybe chain instead of combine ?

85

first part of this sentence doesn't say much, maybe drop it?

91

doesn't seem to be implemented anywhere? I suppose it is just a convenience wrapper around Provider::fromYAMLFile, is it worth it?

[sammccall]

clang-tools-extra/clangd/ConfigProvider.cpp

40

Tracing isn't trivially cheap: that usually involves taking a mutex, writing to a buffer, and I think flushing it to disk.

So we shouldn't trace things that are both hot and small, which I think this is.

I added a tracer to Provider::getConfig - OK to revisit if we need something more fine-grained?

42

Yeah, this is a good point.
One possible design would be to have a "latency-sensitivity" setting, probably on Params, and not bothering to stat the file if we're highly sensitive. (i.e. always reuse the cached value unless the cache is uninitialized).

I've added this idea to the class comment.

Related: I think an abstraction like this, with some thought put into invalidation policy, should be eventually used for .clang-format, .clang-tidy, compile_commands.json, as we should be able to get fresh results with a fairly tiny amount of IO.

58

I considered doing nothing here, or setting Size = Buf->size.

But there are *possible* scenarios where you go back to the old state (think switching git branches) and then end up with a poisoned cache.

And this is cheap (simple, and also doesn't actually cause you to miss the cache when you want to hit it)

68

scope_exit is a neat idea, but when I tried it I found the code hard to reason about.

I extracted a function instead.

Unnesting this caused me to catch a case i'd missed: if stat succeeds but read fails (once), I'd prefer to retry the read before caching the failure forever... (again, the branch switching workflows)

121

No, it's documented that Component isn't necessarily part of the path.
The only case I could find of this actually happening is that "/foo/bar/" iterates as "foo", "bar", ".", at least in some cases.

Extended the comment a bit.

182

the fact that this is Fragment instead of Fragment(P, C)?

Yeah, this is an argument for adding an interface rather than using std::function.
But I don't think it's a big deal or really actionable in this patch.

clang-tools-extra/clangd/ConfigProvider.h

85

I think just saying "this function must be threadsafe" is a bit confusing because it's a const method, it's like saying "this function does not throw exceptions" in llvm code.

But I shoud have made my point more explicit. Replaced with "Despite the need for caching..."

91

Whoops, this was copy/pasted from a prototype.
I decided Provider::fromYAMLFile was a better name.

[sammccall]

address comments

[sammccall]

Try to get the tests to pass on windows.

[kadircet]

clang-tools-extra/clangd/ConfigProvider.cpp

39

why do we want to cache failure case for missing files ?

If it is truly missing and we didn't race with another process (likely git-checkout), the next stat will fail and we'll return a no-op fragment.
If it was a race, we want to pick it up on the next run (and reading twice might not have enough of a delay in between), so why not just cache with a sentinel key in here too?

[sammccall]

Don't cache failed opens.

clang-tools-extra/clangd/ConfigProvider.cpp

39

Yeah I was worried about a file we can stat but chronically can't read (e.g. because permissions are wrong). But this is probably not worth worrying about. Changed no not cache as you suggest.

[kadircet]

thanks LGTM!

clang-tools-extra/clangd/CMakeLists.txt

add_clang_library(clangDaemon
   ClangdServer.cpp
   CodeComplete.cpp
   CodeCompletionStrings.cpp
   CollectMacros.cpp
   CompileCommands.cpp
   Compiler.cpp
   Config.cpp
   ConfigCompile.cpp
+  ConfigProvider.cpp
   ConfigYAML.cpp
   Diagnostics.cpp
   DraftStore.cpp
   ExpectedTypes.cpp
   FindSymbols.cpp
   FindTarget.cpp
   FileDistance.cpp
   Format.cpp

clang-tools-extra/clangd/ConfigProvider.h

 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/SourceMgr.h"
 #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;
 };
 
 /// 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.
 /// Typically this is obtained from a Fragment by calling Fragment::compile().
 ///
 /// Calling it updates the configuration to reflect settings from the fragment.
 /// Returns true if the condition was met and the settings were used.
 using CompiledFragment = std::function<bool(const Params &, Config &)>;
 
+/// A source of configuration fragments.
+/// Generally these providers reflect a fixed policy for obtaining config,
+/// but return different concrete configuration over time.
+/// e.g. a provider that reads config from files is responsive to file changes.
+class Provider {
+public:
+  virtual ~Provider() = default;
+
+  // Reads fragments from a single YAML file with a fixed path.
+  static std::unique_ptr<Provider> fromYAMLFile(llvm::StringRef AbsPathPath,
+                                                const ThreadsafeFS &);
+  // Reads fragments from YAML files found relative to ancestors of Params.Path.
+  //
+  // All fragments that exist are returned, starting from distant ancestors.
+  // For instance, given RelPath of ".clangd", then for source file /foo/bar.cc,
+  // the searched fragments are [/.clangd, /foo/.clangd].
+  //
+  // If Params does not specify a path, no fragments are returned.
+  static std::unique_ptr<Provider>
+  fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath, const ThreadsafeFS &);
+
+  /// A provider that includes fragments from all the supplied providers.
+  /// Order is preserved; later providers take precedence over earlier ones.
+  static std::unique_ptr<Provider>
+      combine(std::vector<std::unique_ptr<Provider>>);
+
+  /// Build a config based on this provider.
+  Config getConfig(const Params &, DiagnosticCallback) const;
+
+private:
+  /// Provide fragments that may be relevant to the file.
+  /// The configuration provider is not responsible for testing conditions.
+  ///
+  /// Providers are expected to cache compiled fragments, and only
+  /// reparse/recompile when the source data has changed.
+  /// Despite the need for caching, this function must be threadsafe.
+  ///
+  /// When parsing/compiling, the DiagnosticCallback is used to report errors.
+  virtual std::vector<CompiledFragment>
+  getFragments(const Params &, DiagnosticCallback) const = 0;
+};
+
 } // namespace config
 } // namespace clangd
 } // namespace clang
 
 #endif

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/StringMap.h"
+#include "llvm/Support/Path.h"
+#include <mutex>
+
+namespace clang {
+namespace clangd {
+namespace config {
+
+// Threadsafe cache around reading a YAML config file from disk.
+class FileConfigCache {
+  std::mutex Mu;
+  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();
+    CachedValue.clear();
+
+    auto Buf = FS.getBufferForFile(Path);
+    // If stat() succeeds but we failed to read, retry once and cache failure.
+    if (!Buf)
+      Buf = FS.getBufferForFile(Path);
+    if (!Buf)
+      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).
+    if (Buf->get()->getBufferSize() != Size) {
+      Size = -1;
+      MTime = {};
+    }
+
+    // Finally parse and compile the actual fragments.
+    for (auto &Fragment :
+         Fragment::parseYAML(Buf->get()->getBuffer(), Path, DC))
+      CachedValue.push_back(std::move(Fragment).compile(DC));
+  }
+
+public:
+  // Must be set before the cache is used. Not a constructor param to allow
+  // computing ancestor-relative paths to be deferred.
+  std::string Path;
+
+  // 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,
+            std::vector<CompiledFragment> &Out) {
+    assert(llvm::sys::path::is_absolute(Path));
+    auto FS = TFS.view(/*CWD=*/llvm::None);
+    auto Stat = FS->status(Path);
+    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.
+      return;
+    }
+
+    std::lock_guard<std::mutex> Lock(Mu);
+    updateCacheLocked(*Stat, *FS, DC);
+    llvm::copy(CachedValue, std::back_inserter(Out));
+  }
+};
+
+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);
+      return Result;
+    };
+
+  public:
+    AbsFileProvider(llvm::StringRef Path, const ThreadsafeFS &FS) : FS(FS) {
+      assert(llvm::sys::path::is_absolute(Path));
+      Cache.Path = Path.str();
+    }
+  };
+
+  return std::make_unique<AbsFileProvider>(AbsPath, FS);
+}
+
+std::unique_ptr<Provider>
+Provider::fromAncestorRelativeYAMLFiles(llvm::StringRef RelPath,
+                                        const ThreadsafeFS &FS) {
+  class RelFileProvider : public Provider {
+    std::string RelPath;
+    const ThreadsafeFS &FS;
+
+    mutable std::mutex Mu;
+    // Keys are the ancestor directory, not the actual config path within it.
+    // We only insert into this map, so pointers to values are stable forever.
+    // Mutex guards the map itself, not the values (which are threadsafe).
+    mutable llvm::StringMap<FileConfigCache> Cache;
+
+    std::vector<CompiledFragment>
+    getFragments(const Params &P, DiagnosticCallback DC) const override {
+      namespace path = llvm::sys::path;
+
+      if (P.Path.empty())
+        return {};
+
+      // Compute absolute paths to all ancestors (substrings of P.Path).
+      llvm::StringRef Parent = path::parent_path(P.Path);
+      llvm::SmallVector<llvm::StringRef, 8> Ancestors;
+      for (auto I = path::begin(Parent, path::Style::posix),
+                E = path::end(Parent);
+           I != E; ++I) {
+        // Avoid weird non-substring cases like phantom "." components.
+        // In practice, Component is a substring for all "normal" ancestors.
+        if (I->end() < Parent.begin() && I->end() > Parent.end())
+          continue;
+        Ancestors.emplace_back(Parent.begin(), I->end() - Parent.begin());
+      }
+      // Ensure corresponding cache entries exist in the map.
+      llvm::SmallVector<FileConfigCache *, 8> Caches;
+      {
+        std::lock_guard<std::mutex> Lock(Mu);
+        for (llvm::StringRef Ancestor : Ancestors) {
+          auto R = Cache.try_emplace(Ancestor);
+          // Assemble the actual config file path only once.
+          if (R.second) {
+            llvm::SmallString<256> ConfigPath = Ancestor;
+            path::append(ConfigPath, RelPath);
+            R.first->second.Path = ConfigPath.str().str();
+          }
+          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);
+      return Result;
+    };
+
+  public:
+    RelFileProvider(llvm::StringRef RelPath, const ThreadsafeFS &FS)
+        : RelPath(RelPath), FS(FS) {
+      assert(llvm::sys::path::is_relative(RelPath));
+    }
+  };
+
+  return std::make_unique<RelFileProvider>(RelPath, FS);
+}
+
+std::unique_ptr<Provider>
+Provider::combine(std::vector<std::unique_ptr<Provider>> Providers) {
+  struct CombinedProvider : Provider {
+    std::vector<std::unique_ptr<Provider>> Providers;
+
+    std::vector<CompiledFragment>
+    getFragments(const Params &P, DiagnosticCallback DC) const override {
+      std::vector<CompiledFragment> Result;
+      for (const auto &Provider : Providers) {
+        for (auto &Fragment : Provider->getFragments(P, DC))
+          Result.push_back(std::move(Fragment));
+      }
+      return Result;
+    }
+  };
+  auto Result = std::make_unique<CombinedProvider>();
+  Result->Providers = std::move(Providers);
+  return Result;
+}
+
+Config Provider::getConfig(const Params &P, DiagnosticCallback DC) const {
+  trace::Span Tracer("getConfig");
+  if (!P.Path.empty())
+    SPAN_ATTACH(Tracer, "path", P.Path);
+  Config C;
+  for (const auto &Fragment : getFragments(P, DC))
+    Fragment(P, C);
+  return C;
+}
+
+} // namespace config
+} // namespace clangd
+} // namespace clang

clang-tools-extra/clangd/unittests/CMakeLists.txt

add_unittest(ClangdUnitTests ClangdTests
   ClangdTests.cpp
   ClangdLSPServerTests.cpp
   CodeCompleteTests.cpp
   CodeCompletionStringsTests.cpp
   CollectMacrosTests.cpp
   CompileCommandsTests.cpp
   CompilerTests.cpp
   ConfigCompileTests.cpp
+  ConfigProviderTests.cpp
   ConfigYAMLTests.cpp
   DexTests.cpp
   DiagnosticsTests.cpp
   DraftStoreTests.cpp
   ExpectedTypeTest.cpp
   FileDistanceTests.cpp
   FileIndexTests.cpp
   FindSymbolsTests.cpp

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 <atomic>
+
+namespace clang {
+namespace clangd {
+namespace config {
+namespace {
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+
+// Provider that appends an arg to compile flags.
+// The arg is prefix<N>, where N is the times getFragments() was called.
+// It also yields a diagnostic each time it's called.
+class FakeProvider : public Provider {
+  std::string Prefix;
+  mutable std::atomic<unsigned> Index = {0};
+
+  std::vector<CompiledFragment>
+  getFragments(const Params &, DiagnosticCallback DC) const override {
+    DC(llvm::SMDiagnostic("", llvm::SourceMgr::DK_Error, Prefix));
+    CompiledFragment F =
+        [Arg(Prefix + std::to_string(++Index))](const Params &P, Config &C) {
+          C.CompileFlags.Edits.push_back(
+              [Arg](std::vector<std::string> &Argv) { Argv.push_back(Arg); });
+          return true;
+        };
+    return {F};
+  }
+
+public:
+  FakeProvider(llvm::StringRef Prefix) : Prefix(Prefix) {}
+};
+
+std::vector<std::string> getAddedArgs(Config &C) {
+  std::vector<std::string> Argv;
+  for (auto &Edit : C.CompileFlags.Edits)
+    Edit(Argv);
+  return Argv;
+}
+
+// The provider from combine() should invoke its providers in order, and not
+// cache their results.
+TEST(ProviderTest, Combine) {
+  CapturedDiags Diags;
+  std::vector<std::unique_ptr<Provider>> Providers;
+  Providers.push_back(std::make_unique<FakeProvider>("foo"));
+  Providers.push_back(std::make_unique<FakeProvider>("bar"));
+  auto Combined = Provider::combine(std::move(Providers));
+  Config Cfg = Combined->getConfig(Params(), Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics,
+              ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+  EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo1", "bar1"));
+  Diags.Diagnostics.clear();
+
+  Cfg = Combined->getConfig(Params(), Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics,
+              ElementsAre(DiagMessage("foo"), DiagMessage("bar")));
+  EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo2", "bar2"));
+}
+
+const char *AddFooWithErr = R"yaml(
+CompileFlags:
+  Add: foo
+  Unknown: 42
+)yaml";
+
+const char *AddBarBaz = R"yaml(
+CompileFlags:
+  Add: bar
+---
+CompileFlags:
+  Add: baz
+)yaml";
+
+TEST(ProviderTest, FromYAMLFile) {
+  MockFS FS;
+  FS.Files["foo.yaml"] = AddFooWithErr;
+
+  CapturedDiags Diags;
+  auto P = Provider::fromYAMLFile(testPath("foo.yaml"), FS);
+  auto Cfg = P->getConfig(Params(), Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics,
+              ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+  Diags.Diagnostics.clear();
+
+  Cfg = P->getConfig(Params(), Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached, not re-parsed";
+  EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo"));
+
+  FS.Files["foo.yaml"] = AddBarBaz;
+  Cfg = P->getConfig(Params(), Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "New config, no errors";
+  EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("bar", "baz"));
+
+  FS.Files.erase("foo.yaml");
+  Cfg = P->getConfig(Params(), Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Missing file is not an error";
+  EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
+}
+
+TEST(ProviderTest, FromAncestorRelativeYAMLFiles) {
+  MockFS FS;
+  FS.Files["a/b/c/foo.yaml"] = AddBarBaz;
+  FS.Files["a/foo.yaml"] = AddFooWithErr;
+
+  std::string ABCPath = testPath("a/b/c/d/test.cc");
+  Params ABCParams;
+  ABCParams.Path = ABCPath;
+  std::string APath = testPath("a/b/e/f/test.cc");
+  Params AParams;
+  AParams.Path = APath;
+
+  CapturedDiags Diags;
+  auto P = Provider::fromAncestorRelativeYAMLFiles("foo.yaml", FS);
+
+  auto Cfg = P->getConfig(Params(), Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty());
+  EXPECT_THAT(getAddedArgs(Cfg), IsEmpty());
+
+  Cfg = P->getConfig(ABCParams, Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics,
+              ElementsAre(DiagMessage("Unknown CompileFlags key Unknown")));
+  EXPECT_THAT(getAddedArgs(Cfg), ElementsAre("foo", "bar", "baz"));
+  Diags.Diagnostics.clear();
+
+  Cfg = P->getConfig(AParams, Diags.callback());
+  EXPECT_THAT(Diags.Diagnostics, IsEmpty()) << "Cached config";
+  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"));
+}
+
+} // namespace
+} // namespace config
+} // namespace clangd
+} // namespace clang