[clangd] Add an option controlling caching of compile commands.

[ilya-biryukov]

Summary

Disabled by default and hidden, caching for most implementation
already happens outside clangd, so we rarely need to change it.

[ilya-biryukov]

Testing should be possible with lit tests, I'll look into that. Let me know if there's anything else about this patch that needs attention. Thanks!

[sammccall]

Generally LG.
I guess you might be able to test this by starting with a/compile_flags.txt and a/b/x.cc, and then adding a/b/compile_flags.txt?

clangd/ClangdLSPServer.h

38

(the options split looks a little odd from the outside. One could make an argument for inheriting ClangdLSPServer::Options from ClangdServer::Options and adding the compile-commands/code completion options there. No need to restructure anything in this patch though)

105

nit: any reason for unique_ptr over optional?
(With optional, I think it's clear enough to remove the comment)

clangd/tool/ClangdMain.cpp

141

init(true) to avoid changing behavior?

[ilya-biryukov]

clangd/ClangdLSPServer.h

38

Options struct SG, even though is number of params is manageable, and we have just a single call of this ctor at the time.
I'm not a big fan of inheriting data structs, though, would rather use composition.

105

CachingCompilationDb is non-movable (because of std::mutex field), so we can't properly initialize it in ctor-initializers.
And we need it to pass it into ClangdServer in ctor-initializers, so it's hard to put the init code into the ctor body.
That all looks brittle, but I wouldn't refactor this in this patch.

clangd/tool/ClangdMain.cpp

141

This is intentional, it seems false is a better default, given that all implementation in LLVM cache it all themselves.
We should set to true for our CDB that actually needs it. WDYT?

[sammccall]

clangd/tool/ClangdMain.cpp

141

I agree the default should be false, at least eventually.

I'm not sure what you're suggesting though - we change the default value of the flag in our own copy when we link against our DB that doesn't cache?

That could work. I wonder if this will break others who might have similar DBs.

[ilya-biryukov]

clangd/tool/ClangdMain.cpp

141

I suggested telling our users to set this flag to true.
Otherwise I think we should just punt on this change and not introduce the flag. If we'll have dependency tracking, caching could probably be nicely incorporated into the build system integration and than we'll just remove the caching from ClangdLSPServer.
Happy to go this way, BTW, I don't think this cache creates any problems apart from some inefficiency.

The flag is of no use if we're the only ones who need to set it to 'true', but 'true' is also the default.

I wonder if this will break others who might have similar DBs.

Do we know of anyone else who builds clangd with custom DB implementations?
If there are any and this change breaks them, we can revert it.

[malaperle]

Does this change affect the switching of compilation database, through workspace/didChangeConfiguration ?

[ilya-biryukov]

A recent change (D49267) is another indication that caching might be doing more wrong than good. I assume the caching does not give us much performance-wise, we only request compile commands for file reparses and reparses tend to be slow and do lots of file system accesses anyway.
Maybe we should just disable it altogether. @sammccall, @simark, WDYT?

[sammccall]

In D48071#1171289, @ilya-biryukov wrote:

A recent change (D49267) is another indication that caching might be doing more wrong than good. I assume the caching does not give us much performance-wise, we only request compile commands for file reparses and reparses tend to be slow and do lots of file system accesses anyway.
Maybe we should just disable it altogether. @sammccall, @simark, WDYT?

Yeah, the caching is bad and we should get rid of it. I thought an option would make this smoother but I no longer think so.

AFAIK the only people who actually need this option at present are internal google users due to our suspicious CDB.
Those people aren't well-served by having to set an option. Let's find another solution for that and then just remove the caching?

clangd/ClangdLSPServer.h

 /// dispatch and ClangdServer together.
 class ClangdLSPServer : private DiagnosticsConsumer, private ProtocolCallbacks {
 public:
   /// If \p CompileCommandsDir has a value, compile_commands.json will be
   /// loaded only from \p CompileCommandsDir. Otherwise, clangd will look
   /// for compile_commands.json in all parent directories of each file.
   ClangdLSPServer(JSONOutput &Out, const clangd::CodeCompleteOptions &CCOpts,
                   llvm::Optional<Path> CompileCommandsDir,
-                  const ClangdServer::Options &Opts);
+                  const ClangdServer::Options &Opts, bool CacheCompileCommands);
 
   /// Run LSP server loop, receiving input for it from \p In. \p In must be
   /// opened in binary mode. Output will be written using Out variable passed to
   /// class constructor. This method must not be executed more than once for
   /// each instance of ClangdLSPServer.
   ///
   /// \return Whether we received a 'shutdown' request before an 'exit' request.
   bool run(std::FILE *In,
private:
   typedef std::map<clangd::Diagnostic, std::vector<Fix>, LSPDiagnosticCompare>
       DiagnosticToReplacementMap;
   /// Caches FixIts per file and diagnostics
   llvm::StringMap<DiagnosticToReplacementMap> FixItsMap;
 
   // Various ClangdServer parameters go here. It's important they're created
   // before ClangdServer.
   DirectoryBasedGlobalCompilationDatabase NonCachedCDB;
-  CachingCompilationDb CDB;
+  // Can be null if no caching was requested.
+  std::unique_ptr<CachingCompilationDb> CachedCDB;
 
   RealFileSystemProvider FSProvider;
   /// Options used for code completion
   clangd::CodeCompleteOptions CCOpts;
   /// The supported kinds of the client.
   SymbolKindBitset SupportedSymbolKinds;
 
   // Store of the current versions of the open documents.

clangd/ClangdLSPServer.cpp

 //===--- ClangdLSPServer.cpp - LSP server ------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
 #include "Diagnostics.h"
 #include "JSONRPCDispatcher.h"
 #include "SourceCode.h"
 #include "URI.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 
 using namespace clang::clangd;
 using namespace clang;
 
 namespace {

 
 void ClangdLSPServer::onExit(ExitParams &Params) { IsDone = true; }
 
 void ClangdLSPServer::onDocumentDidOpen(DidOpenTextDocumentParams &Params) {
   PathRef File = Params.textDocument.uri.file();
   if (Params.metadata && !Params.metadata->extraFlags.empty()) {
     NonCachedCDB.setExtraFlagsForFile(File,
                                       std::move(Params.metadata->extraFlags));
-    CDB.invalidate(File);
+    if (CachedCDB)
+      CachedCDB->invalidate(File);
   }
 
   std::string &Contents = Params.textDocument.text;
 
   DraftMgr.addDraft(File, Contents);
   Server.addDocument(File, Contents, WantDiagnostics::Yes);
 }
 
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
   auto WantDiags = WantDiagnostics::Auto;
   if (Params.wantDiagnostics.hasValue())
     WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Yes
                                                   : WantDiagnostics::No;
 
   PathRef File = Params.textDocument.uri.file();
   llvm::Expected<std::string> Contents =
       DraftMgr.updateDraft(File, Params.contentChanges);
   if (!Contents) {
     // If this fails, we are most likely going to be not in sync anymore with
     // the client.  It is better to remove the draft and let further operations
     // fail rather than giving wrong results.
     DraftMgr.removeDraft(File);
     Server.removeDocument(File);
-    CDB.invalidate(File);
+    if (CachedCDB)
+      CachedCDB->invalidate(File);
     log(llvm::toString(Contents.takeError()));
     return;
   }
 
   Server.addDocument(File, *Contents, WantDiags);
 }
 
 void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams &Params) {

 void ClangdLSPServer::onChangeConfiguration(
     DidChangeConfigurationParams &Params) {
   ClangdConfigurationParamsChange &Settings = Params.settings;
 
   // Compilation database change.
   if (Settings.compilationDatabasePath.hasValue()) {
     NonCachedCDB.setCompileCommandsDir(
         Settings.compilationDatabasePath.getValue());
-    CDB.clear();
+    if (CachedCDB)
+      CachedCDB->clear();
 
     reparseOpenedFiles();
   }
 }
 
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
                                  const clangd::CodeCompleteOptions &CCOpts,
                                  llvm::Optional<Path> CompileCommandsDir,
-                                 const ClangdServer::Options &Opts)
-    : Out(Out), NonCachedCDB(std::move(CompileCommandsDir)), CDB(NonCachedCDB),
+                                 const ClangdServer::Options &Opts,
+                                 bool CacheCompileCommands)
+    : Out(Out), NonCachedCDB(std::move(CompileCommandsDir)),
+      CachedCDB(CacheCompileCommands
+                    ? llvm::make_unique<CachingCompilationDb>(NonCachedCDB)
+                    : nullptr),
       CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()),
-      Server(CDB, FSProvider, /*DiagConsumer=*/*this, Opts) {}
+      Server(CachedCDB ? (GlobalCompilationDatabase&) *CachedCDB : NonCachedCDB, FSProvider,
+             /*DiagConsumer=*/*this, Opts) {}
 
 bool ClangdLSPServer::run(std::FILE *In, JSONStreamStyle InputStyle) {
   assert(!IsDone && "Run was called before");
 
   // Set up JSONRPCDispatcher.
   JSONRPCDispatcher Dispatcher([](const json::Expr &Params) {
     replyError(ErrorCode::MethodNotFound, "method not found");
   });

clangd/tool/ClangdMain.cpp

static llvm::cl::opt<Path> YamlSymbolFile(
     "yaml-symbol-file",
     llvm::cl::desc(
         "YAML-format global symbol file to build the static index. Clangd will "
         "use the static index for global code completion.\n"
         "WARNING: This option is experimental only, and will be removed "
         "eventually. Don't rely on it."),
     llvm::cl::init(""), llvm::cl::Hidden);
 
+static llvm::cl::opt<bool> CacheCompilationArgs(
+    "cache-compilation-args",
+    llvm::cl::desc("When true, clangd will cache compilation arguments that "
+                   "come from the compilation databases."),
+    llvm::cl::init(false), llvm::cl::Hidden);
+
 int main(int argc, char *argv[]) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::cl::ParseCommandLineOptions(argc, argv, "clangd");
   if (Test) {
     RunSynchronously = true;
     InputStyle = JSONStreamStyle::Delimited;
     PrettyPrint = true;
   }
int main(int argc, char *argv[]) {
   }
   Opts.AsyncThreadsCount = WorkerThreadsCount;
 
   clangd::CodeCompleteOptions CCOpts;
   CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
   CCOpts.Limit = LimitResults;
 
   // Initialize and run ClangdLSPServer.
-  ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath, Opts);
+  ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath, Opts,
+                            CacheCompilationArgs);
   constexpr int NoShutdownRequestErrorCode = 1;
   llvm::set_thread_name("clangd.main");
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
   return LSPServer.run(stdin, InputStyle) ? 0 : NoShutdownRequestErrorCode;
 }