[PATCH] D97417: [clangd] use a compatible preamble for the first AST built

Tue Mar 2 14:25:28 PST 2021

qchateau added a comment.

I am much more afraid of providing bad results than I am afraid of degrading performance. I mean, eventually the "real" preamble is built, and the results are just as correct as before, but it may yield incorrect results until we invalidate the AST.
That is especially the case as I'm not very experienced in compiler design and I don't have a deep understanding of what happens behind the curtains when using a PCH.
There are probably cases where we degrade perf, but overall my experience with this patch is overwhelmingly positive, especially for "long coding sessions".

> I am afraid this can happen more than twice

I'm not sure I follow, I see these different situations (in TUScheduler):

- runWithPreamble
  - PreambleConsistency::Stale -> will wait for a "real" preamble, no change in behavior
  - PreambleConsistency::StaleOrAbsent
    - a preamble is available, we'll use it, no change in behavior
    - the preamble is not yet built, we'll use a compatible preamble instead -> this means instant partial completion !
- runWithAST
  - a preamble is available, we'll use it, no change in behavior
  - the preamble is not yet built, we'll use a compatible preamble to start building the AST straight away -> here there is a risk of double work, if the compatible preamble is useless, we'll start building the AST *while* we're also building the preamble

But I don't see how we can do worse than building the AST and the preamble at the same time ?

> could you give a bit more information on average preamble build latency in the files you are testing this with

Extracts from VSCode logs, between the file being opened and the semantic token request completion.
Also worth noting that we have partial completion with 0 delay (with the compatible preamble)

Without the patch:

  I[22:40:57.317] ASTWorker building file /home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.h version 1 with command inferred from /home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.cpp
  I[22:41:07.893] --> reply:textDocument/semanticTokens/full(1) 9921 ms

  I[22:41:16.526] ASTWorker building file /home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.cpp version 1 with command
  I[22:41:38.401] --> reply:textDocument/semanticTokens/full(12) 21877 ms

  I[22:41:53.861] ASTWorker building file /home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.h version 1 with command inferred from /home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.cpp
  I[22:42:06.006] --> reply:textDocument/semanticTokens/full(23) 12144 ms

  I[22:42:08.619] ASTWorker building file /home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.cpp version 1 with command
  I[22:42:21.920] --> reply:textDocument/semanticTokens/full(28) 13278 ms

With the patch

  I[22:36:32.828] ASTWorker building file /home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.h version 1 with command inferred from /home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.cpp
  I[22:36:43.211] --> reply:textDocument/semanticTokens/full(3) 9675 ms

  I[22:36:52.331] ASTWorker building file /home/quentin/dev/llvm-project/clang-tools-extra/clangd/ParsedAST.cpp version 1 with command
  I[22:36:58.206] --> reply:textDocument/semanticTokens/full(9) 5881 ms

  I[22:37:13.474] ASTWorker building file /home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.h version 1 with command inferred from /home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.cpp
  I[22:37:15.840] --> reply:textDocument/semanticTokens/full(15) 2351 ms

  I[22:37:23.939] ASTWorker building file /home/quentin/dev/llvm-project/clang-tools-extra/clangd/IncludeFixer.cpp version 1 with command
  I[22:37:25.030] --> reply:textDocument/semanticTokens/full(20) 1089 ms

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97417