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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
