[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks for your patience!
This is very nice, only +100 lines on TUScheduler.cpp in the end!



================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:224
+              std::vector<Diag> CIDiags, WantDiagnostics WantDiags) {
     // Make possibly expensive copy while not holding the lock.
+    Request Req = {std::move(CI), std::move(PI), std::move(CIDiags), WantDiags,
----------------
comment is now obsolete, remove it and consider inlining `Req`


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:325
   mutable std::condition_variable ReqCV;           /* GUARDED_BY(Mutex) */
   std::shared_ptr<const PreambleData> LatestBuild; /* GUARDED_BY(Mutex) */
 
----------------
this is no longer locked or concurrently accessed right?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:378
+  /// Used to inform ASTWorker about a new preamble build by PreambleThread.
+  /// Diagnostics are only published through this callback, which ensures they
+  /// are always for newer versions of the file. As this callback gets called in
----------------
nit: last sentence is a fragment, I think you want to swap the dot and comma:

...through this callback. This ensures... of the file, as the callback...


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:408
+
+  /// Returns the latest built preamble, might be null if no preamble has been
+  /// built or latest attempt resulted in a failure.
----------------
Unused, I think?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:793
+  // Used to check whether we can update AST cache.
+  bool InputsAreTheSame =
+      std::tie(FileInputs->CompileCommand, FileInputs->Contents) ==
----------------
I'd suggest InputsAreLatest or something that hints at *why* they might differ


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:810
+  });
+  llvm::Optional<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
+  if (!AST) {
----------------
This is reusing the AST built for a read if the file (and preamble) hasn't changed, right?

I think it's helpful to explain where teh cache data might be coming from.

IIRC this is the case we thought wasn't so important. It competes with a different optimization: skipping clang-tidy, warning analysis, etc when building ASTs because we know their diagnostics will never be used.

This might be a good place to leave a comment like "FIXME: maybe we're better off never reusing this AST, so queued AST builds aren't required to produce diagnostics" or so


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:862
+  // Stash the AST in the cache for further use if it was build for current
+  // inputs.
+  if (InputsAreTheSame) {
----------------
This explains the what but not the why.
(We may be building an older version of the source, as the queue raced ahead while we were waiting on the preamble. In that case the queue can't reuse the AST we built.)
Comment could go here or on InputsAreTheSame


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76725