[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

> if/when we have a native implementation, supporting multiple mechanisms with different layering requirements to get at most a 2x win in watcher resource usage seems like a dubious way to spend our complexity budget.

I guess the only think that might force us to do this is the low system limit on the number of file watches. I do remember rumors that it can be a problem in, but I haven't really seen those on my own, so maybe I'm just pretending.
But I do agree that we shouldn't commit to have yet-another implementation in the long-term, unless we have good justification for it.

> it looks like the current design throws away all compilation databases (via the global DB) when *any* compile_commands.json changes. I think it's important that other stateful compilation databases aren't thrown away for unrelated reasons. e.g. the Bazel DB takes nontrivial time to reinitialize.

Good point, we should fix this. Started a thread about it in the inline comments.



================
Comment at: clangd/ClangdLSPServer.cpp:430
     CDB.clear();
-
-    reparseOpenedFiles();
+    compileCommandsChangePost(CCChangeData);
   }
----------------
simark wrote:
> malaperle wrote:
> > ilya-biryukov wrote:
> > > Maybe keep the old logic of reparsing all open files? This would make the change way simpler and I don't think we need this extra complexity in the long run, when we have better integration with the build system.
> > > 
> > > ClangdServer will reuse the preamble if compile command didn't change anyway, so reparse will be very fast and shouldn't be affected.
> > > If the compile command does change, we'll retrigger the full rebuild.
> > I think the change is not that complex but brings much added value. About the integration with the build system, there are many build systems out there so I don't think better integration will be useful in many scenarios (plain make, custom builds, etc). This solution is generic enough so that any build system that generates compile_commands.json will be supported in a pretty good way.
> @malaperle also suggested an alternative way of doing it.  Instead of saving the the compile commands in a custom structure before clearing the cache, we could save the compile flags in the `ParsedAST` object.  When some compile_commands.json changes, we can compare the new compile flags with this saved copy.  I think it would make the code a bit more straightforward.
> I think the change is not that complex but brings much added value.
> @malaperle also suggested an alternative way of doing it. Instead of saving the the compile commands in a custom structure before clearing the cache, we could save the compile flags in the ParsedAST object. When some compile_commands.json changes, we can compare the new compile flags with this saved copy. I think it would make the code a bit more straightforward.

The no-op rebuilds in case nothing changes is a good and long overdue optimization, and I agree that `ParsedAST` or `TUScheduler::update` is probably the right place to put it into. Any other benefits we get from snapshotting the compile commands here? Could we make this optimization in a separate change? It does not seem directly related to the file watching changes. Also happy to look at it, finding the right place to do it might involve digging through layers of classes that manage the AST.

> About the integration with the build system, there are many build systems out there so I don't think better integration will be useful in many scenarios (plain make, custom builds, etc). This solution is generic enough so that any build system that generates compile_commands.json will be supported in a pretty good way.
Just to clarify, the "better buildsystem integration" I refer to is not about enabling support for more build systems (though, that would be nice-to-have), it's about building abstractions that better support the current use-cases, i.e. making the connection between the CompiationDatabase and compiler_commands.json more explicit, allowing the track changes in the build system, etc. We think that file-watching will definitely be part of it, but we don't have full design yet.

In any case, we do think that there are improvements to be made both in compile_commands.json and in our internal Bazel integration.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:71
+  std::lock_guard<std::mutex> Lock(Mutex);
+  CompilationDatabases.clear();
+}
----------------
Could we only clear the databases for the folder under which the changed `compile_commands.json` lives?



================
Comment at: clangd/clients/clangd-vscode/src/extension.ts:35
         ['cpp', 'c', 'cc', 'cxx', 'c++', 'm', 'mm', 'h', 'hh', 'hpp', 'hxx', 'inc'].join() + '}';
+    const compileCommandsFilePattern: string = '**/compile_commands.json';
     const clientOptions: vscodelc.LanguageClientOptions = {
----------------
simark wrote:
> ilya-biryukov wrote:
> > These watches apply to the children of the workspace root only, right?
> > E.g., I quite often open the workspace in 'llvm/tools/clang/tools/extra/clangd', would my `compile_commands.json`, that's outside the workspace, be watched for changes?
> You are right, I don't think files outside the workspace would get watched.
> 
> For your use case, we'll need clangd to watch (or ask the client to watch) the file.
Got it. That would be nice to have, but will have to wait for dynamic registration of file watchers...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267