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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D49267#1171932, @simark wrote:

> 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.


Yes, language client, thanks!

>> - 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.

Right, the number of watched files be large. I bet some implementations will have trouble handling the file watching properly. But that's just a guess, we definitely need some data there. File watching seems like a complicated problem and it may happen to be core enough for clangd performance, so I wouldn't bet on every language client doing it in a reliable, performant way if we can solve it reliably in clangd instead. At least we should have a fallback implementation in case the clients don't support it.
But none of that is set in stone, just thinking out loud.

> 
> 
>> 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.
> 
> 
> 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.

You're right, it won't, the file will only get reparsed with the new compile command whenever user modifies it.
The approach is not ideal, but may be a good middle ground before we figure out how we approach file watching in clangd. Note that there are other things that won't force the updates currently, e.g. changes to the included headers won't cause rebuilds of the source files until user modifies them. And those are much more frequent than changes to compile_commands.json, so they seem like a more pressing problem.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267