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

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49267#1171286, @ilya-biryukov wrote:

> Thanks for putting up this change! It can be really annoying that clangd does not pick up the compile commands that got updated.
>
> A few things of the top of my head on why we might want to punt on using the LSP watches:
>
> - File watching may not be implemented in the language server at all. That's certainly the case for our hacked-up ycmd<->lsp bridge.


I guess you mean language client here, not language server.  In our case we already have a client capable of file watching, so it's convenient for us to do that (plus, this is what the LSP specification recommends).  If you want to make clangd capable of watching files natively, I am not against it, but it's not on our critical path.  As long as the file is watched, I'm happy.

Our client does not yet support file watch registration, that's why I did not implement it, but once we upgrade to a client that does, the plan is to make clangd ask the client to watch `**/compile_commands.json`.  Right now, we hardcoded in our client that we want to watch any file called `compile_commands.json`:

https://github.com/theia-ide/theia/pull/2405/commits/fe48e8d63f98edd8792522381111844a58f2cfa1

> - At some point we'll be interested in changes to the headers our sources have included. This would involve a **lot** of watches for a dynamically changing set of files. There's a good chance we'll have to consider using native file watch APIs instead of the LSP methods to handle those (e.g., for performance reasons).

What would be the performance bottleneck?  There would be a lot of watched files fore sure, but very little change events.

> We've been talking about starting to implement a better buildsystem integration lately and taking compile_commands.json changes into account is certainly on the table. We'll probably have some concrete proposals in August/September.
> 
> In the meanwhile, maybe removing the caching of compile commands could fix the problem? I don't have any data on how important the caching is to performance (I would assume it to not be very important, since we only request compile commands on the file changes; completion, findDefinitions, etc reuse the last compile command anyway). 
>  https://reviews.llvm.org/D48071 added an option to disable the caching, it never landed because we didn't agree on the default, but we can make it happen.
>  It looks like a simpler workaround that works on all clients and we can figure out how to properly integrate file watching later. WDYT?

... see answer below, as you have already provided an updated to this ...

In https://reviews.llvm.org/D49267#1171531, @ilya-biryukov wrote:

> @sammccall pointed out that I've been looking at a different layer of caching.
>  Clangd does per-directory (to avoid reloading compilation database multiple times) and per-file (to avoid calling into compilation database multiple times, it's expensive for our internal CDB) caching of compile commands.
>  https://reviews.llvm.org/D48071 allows to get rid of per-file cache, but still keeps the per-directory cache, so this patch alone won't fix the problem.


Indeed.

> We have a somewhat simple fix in mind: for interactive tools, like clangd, the `JSONCompilationDatabase` could be configured to check for changes to `compile_commands.json` and rebuild the compile commands on each call to its methods, if necessary. E.g. we can still keep the per-directory "caching" of `CompilationDatabase`s inside clangd, but CompilationDatabase instances will start returning new compile commands when compile_commands.json is updated.
>  This would mean more FS accesses (and, potentially, json parsing) on `getCompileCommands` and the same instance of `CompilationDatabase` will now be potentially providing different compile commands for the same file on different `getCompileCommands` calls.
>  However,
> 
> - The FS access concern shouldn't be noticeable for the clang tools: parsing/preprocessing of files is usually done after the `getCompileCommands` call, so the overhead of files access to compile_commands.json and json reads should be negligible, compared to the work to process C/C++ files.
> - The fact that const object now changes seems to be a potential source of confusion for the tools and (maybe?) some of them are not prepared for this, so we should probably allow turning this behavior on and off. clangd (and other interactive tools that might be interested in this), would turn it on, while it will be off by default.
> 
>   It seems this could be implemented somewhere around the `JSONCompilationDatabasePlugin` pretty easily. WDYT? I'm also happy to come up with a patch to `JSONCompilationDatabasePlugin`, at this point I'm pretty familiar with it. Unless you think this approach is flawed or want to do it yourself, of course. In any case, please let us know what's your opinion on this.

The important point of this patch is that when some change happens to some `compile_commands.json`, we detect which open file has changed compile flags and therefore needs re-parse.  It's not clear to me how this is achieved with your proposal.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267