[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.
sammccall added a comment. In https://reviews.llvm.org/D48071#1171289, @ilya-biryukov wrote: > A recent change (https://reviews.llvm.org/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? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.
ilya-biryukov added a subscriber: simark. ilya-biryukov added a comment. Herald added a subscriber: arphaman. A recent change (https://reviews.llvm.org/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? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.
malaperle added a comment. Does this change affect the switching of compilation database, through workspace/didChangeConfiguration ? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.
ilya-biryukov added inline comments. Comment at: clangd/tool/ClangdMain.cpp:141 + "come from the compilation databases."), +llvm::cl::init(false), llvm::cl::Hidden); + sammccall wrote: > ilya-biryukov wrote: > > sammccall wrote: > > > init(true) to avoid changing behavior? > > 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? > 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. 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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.
sammccall added inline comments. Comment at: clangd/tool/ClangdMain.cpp:141 + "come from the compilation databases."), +llvm::cl::init(false), llvm::cl::Hidden); + ilya-biryukov wrote: > sammccall wrote: > > init(true) to avoid changing behavior? > 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? 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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.
ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.h:38 llvm::Optional CompileCommandsDir, - const ClangdServer::Options ); + const ClangdServer::Options , bool CacheCompileCommands); sammccall wrote: > (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) 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. Comment at: clangd/ClangdLSPServer.h:105 + // Can be null if no caching was requested. + std::unique_ptr CachedCDB; sammccall wrote: > nit: any reason for unique_ptr over optional? > (With optional, I think it's clear enough to remove the comment) 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. Comment at: clangd/tool/ClangdMain.cpp:141 + "come from the compilation databases."), +llvm::cl::init(false), llvm::cl::Hidden); + sammccall wrote: > init(true) to avoid changing behavior? 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? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.
sammccall added a comment. 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? Comment at: clangd/ClangdLSPServer.h:38 llvm::Optional CompileCommandsDir, - const ClangdServer::Options ); + const ClangdServer::Options , bool CacheCompileCommands); (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) Comment at: clangd/ClangdLSPServer.h:105 + // Can be null if no caching was requested. + std::unique_ptr CachedCDB; nit: any reason for unique_ptr over optional? (With optional, I think it's clear enough to remove the comment) Comment at: clangd/tool/ClangdMain.cpp:141 + "come from the compilation databases."), +llvm::cl::init(false), llvm::cl::Hidden); + init(true) to avoid changing behavior? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.
ilya-biryukov added a comment. 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! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48071 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48071: [clangd] Add an option controlling caching of compile commands.
ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added subscribers: jkorous, MaskRay, ioeric.
Disabled by default and hidden, caching for most implementation
already happens outside clangd, so we rarely need to change it.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48071
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -134,6 +134,12 @@
"eventually. Don't rely on it."),
llvm::cl::init(""), llvm::cl::Hidden);
+static llvm::cl::opt 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");
@@ -235,7 +241,8 @@
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.
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -35,7 +35,7 @@
/// for compile_commands.json in all parent directories of each file.
ClangdLSPServer(JSONOutput , const clangd::CodeCompleteOptions ,
llvm::Optional CompileCommandsDir,
- const ClangdServer::Options );
+ const ClangdServer::Options , 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
@@ -101,7 +101,8 @@
// 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 CachedCDB;
RealFileSystemProvider FSProvider;
/// Options used for code completion
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -12,6 +12,7 @@
#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"
@@ -133,7 +134,8 @@
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 = Params.textDocument.text;
@@ -157,7 +159,8 @@
// 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;
}
@@ -388,19 +391,25 @@
if (Settings.compilationDatabasePath.hasValue()) {
NonCachedCDB.setCompileCommandsDir(
Settings.compilationDatabasePath.getValue());
-CDB.clear();
+if (CachedCDB)
+ CachedCDB->clear();
reparseOpenedFiles();
}
}
ClangdLSPServer::ClangdLSPServer(JSONOutput ,
const clangd::CodeCompleteOptions ,
llvm::Optional CompileCommandsDir,
- const ClangdServer::Options )
-: Out(Out), NonCachedCDB(std::move(CompileCommandsDir)), CDB(NonCachedCDB),
+ const ClangdServer::Options ,
+ bool CacheCompileCommands)
+: Out(Out), NonCachedCDB(std::move(CompileCommandsDir)),
+ CachedCDB(CacheCompileCommands
+? llvm::make_unique(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");
___
cfe-commits mailing list
