[PATCH] D81069: [clangd] Don't build diagnostics when preparing AST for a normal action.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 8 inline comments as done.
sammccall added a comment.

> rarely happens anyway

citation needed.

While this is true today, it seems fairly likely that async preambles is going to flip the dominant AST reuse from diagnostics -> read to read -> diagnostics.

This change would make the builds 20% faster for the cache misses on AST read (~30%), at the cost of rebuilding when diagnostics would hit (~0%).
If those probabilities flip, this becomes a really bad trade.

Let's wait a couple of weeks until we have data from production.



================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:796
       [](std::vector<Diag>) { ADD_FAILURE() << "Should not be called."; });
   S.runWithAST("touchAST", FooCpp, [](Expected<InputsAndAST> IA) {
     // Make sure the AST was actually built.
----------------
kadircet wrote:
> is it worth asserting here (or in a new test) that AST.diags() is empty, whereas it is non-empty below ? (ofc, after introducing some diags to the code)
I don't think we want to *guarantee* when they're present/absent with runWithAST, this is an implementation choice. Added a comment to make this contract explicit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81069/new/

https://reviews.llvm.org/D81069