[clangd] Always use preamble (even stale) for code completion

[ilya-biryukov]

Summary

This improves performance of code completion, because we avoid stating
the files from the preamble and never attempt to parse the files
without using the preamble if it's provided.

However, the change comes at a cost of sometimes providing incorrect
results when doing code completion after making actually considerable
changes to the files used in the preamble or the preamble itself.
Eventually the preamble will get rebuilt and code completion will
be providing the correct results.

[bkramer]

clangd/Compiler.h

38

This comment is somewhat messy now. Can you rephrase it a bit?

[sammccall]

Big +1 to the change here.
Just suggestions for the comments.

clangd/CodeComplete.cpp

533–534

delibirately -> deliberately

Thanks for adding the motivation here! I think this can be a bit terser, but up to you.
e.g.

// We reuse the preamble whether it's valid or not. This is a correctness/performance 
// tradeoff: building without a preamble is slow, and completion is latency-sensitive.

clangd/Compiler.h

38

+1, this has grown a bit unwieldy. Maybe:

/// Creates a compiler which will build the provided file.
/// The preamble will not be checked for validity - caller should do that if needed.
/// The returned compiler's VFS may differ due to command-line flags.
/// The returned value must be consumed by a FrontendAction to avoid leaking MainFile.
/// May return null in error cases.

[ilya-biryukov]

• Rewrote the comment for prepareCompilerInstance
• Call CanReuse() and discard its results. We have clients that rely on having the stat() calls for files from preamble

[ilya-biryukov]

• Use a better comment suggest by Sam

[ilya-biryukov]

clangd/CodeComplete.cpp

533–534

Looks much better, thanks.

clangd/Compiler.h

38

I rewrote the comment before reading your suggestion, it should be better now.
If it's still unclear, let me know.

[ilya-biryukov]

• Fixed a typo

[sammccall]

Thanks, LG!

clangd/Compiler.h

39

nit: "buffer for MainFile" -> "MainFile"
(since MainFile is already a buffer. also: less wrapping!)

[ilya-biryukov]

• s/buffer for MainFile/MainFile/

clangd/CodeComplete.cpp

auto CI = createInvocationFromCommandLine(
                                           &DummyDiagsConsumer, false),
       VFS);
   assert(CI && "Couldn't create CompilerInvocation");
   CI->getFrontendOpts().DisableFree = false;
 
   std::unique_ptr<llvm::MemoryBuffer> ContentsBuffer =
       llvm::MemoryBuffer::getMemBufferCopy(Contents, FileName);
 
-  // Attempt to reuse the PCH from precompiled preamble, if it was built.
+  // Note that we delibirately allow outdated preambles. Not using the preamble
+  // makes code completion much slower and we feel that the right trade-off here
+  // is to trade correctness in some cases (preamble has not been rebuilt after
+  // changes to a file) for performance. Eventually the preamble will be rebuilt
+  // and code completion will produce correct results.
   if (Preamble) {
     auto Bounds =
         ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
-    if (!Preamble->CanReuse(*CI, ContentsBuffer.get(), Bounds, VFS.get()))
-      Preamble = nullptr;
+    // FIXME(ibiryukov): Remove this call to CanReuse() after we'll fix
+    // clients relying on getting stats for preamble files during code
+    // completion.
+    // Note that results of CanReuse() are ignored, see the comment above.
+    Preamble->CanReuse(*CI, ContentsBuffer.get(), Bounds, VFS.get());
   }
-
   auto Clang = prepareCompilerInstance(
       std::move(CI), Preamble, std::move(ContentsBuffer), std::move(PCHs),
       std::move(VFS), DummyDiagsConsumer);
   auto &DiagOpts = Clang->getDiagnosticOpts();
   DiagOpts.IgnoreWarnings = true;
 
   auto &FrontendOpts = Clang->getFrontendOpts();
   FrontendOpts.SkipFunctionBodies = true;
   FrontendOpts.CodeCompleteOpts = Options;

clangd/Compiler.h

 namespace clangd {
 
 class IgnoreDiagnostics : public DiagnosticConsumer {
 public:
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override {}
 };
 
-/// Creates a CompilerInstance with the main file contens overridden.
-/// The preamble will be reused unless it is null.
-/// Note that the vfs::FileSystem inside returned instance may differ if
-/// additional file remappings occur in command-line arguments.
-/// On some errors, returns null. When non-null value is returned, it's expected
-/// to be consumed by the FrontendAction as it will have a pointer to the
-/// MainFile buffer that will only be deleted if BeginSourceFile is called.
+/// Creates a compiler instance, configured so that:
+///   - Contents of the parsed file are remapped to \p MainFile.
+///   - Preamble is overriden to use PCH passed to this function. It means the
+///     changes to the preamble headers or files included in the preamble are
+///     not visible to this compiler instance.
+///   - vfs::FileSystem is used for all underlying file accesses. The actual
+///     vfs used by the compiler may be an overlay over the passe vfs.
+/// Returns null on errors. When non-null value is returned, it is expected to
+/// be consumed by FrontendAction::BeginSourceFile to properly destroy buffer
+/// for \p MainFile.
 std::unique_ptr<CompilerInstance> prepareCompilerInstance(
     std::unique_ptr<clang::CompilerInvocation>, const PrecompiledPreamble *,
     std::unique_ptr<llvm::MemoryBuffer> MainFile,
     std::shared_ptr<PCHContainerOperations>,
     IntrusiveRefCntPtr<vfs::FileSystem>, DiagnosticConsumer &);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clangd/Compiler.cpp

 //===--- Compiler.cpp -------------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "Compiler.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Lex/PreprocessorOptions.h"
 
 namespace clang {
 namespace clangd {
 
-/// Creates a CompilerInstance from \p CI, with main buffer overriden to \p
-/// Buffer and arguments to read the PCH from \p Preamble, if \p Preamble is not
-/// null. Note that vfs::FileSystem inside returned instance may differ from \p
-/// VFS if additional file remapping were set in command-line arguments.
-/// On some errors, returns null. When non-null value is returned, it's expected
-/// to be consumed by the FrontendAction as it will have a pointer to the \p
-/// Buffer that will only be deleted if BeginSourceFile is called.
 std::unique_ptr<CompilerInstance>
 prepareCompilerInstance(std::unique_ptr<clang::CompilerInvocation> CI,
                         const PrecompiledPreamble *Preamble,
                         std::unique_ptr<llvm::MemoryBuffer> Buffer,
                         std::shared_ptr<PCHContainerOperations> PCHs,
                         IntrusiveRefCntPtr<vfs::FileSystem> VFS,
                         DiagnosticConsumer &DiagsClient) {
   assert(VFS && "VFS is null");
   assert(!CI->getPreprocessorOpts().RetainRemappedFileBuffers &&
          "Setting RetainRemappedFileBuffers to true will cause a memory leak "
          "of ContentsBuffer");
 
   // NOTE: we use Buffer.get() when adding remapped files, so we have to make
   // sure it will be released if no error is emitted.
   if (Preamble) {
-    Preamble->AddImplicitPreamble(*CI, VFS, Buffer.get());
+    Preamble->OverridePreamble(*CI, VFS, Buffer.get());
   } else {
     CI->getPreprocessorOpts().addRemappedFile(
         CI->getFrontendOpts().Inputs[0].getFile(), Buffer.get());
   }
 
   auto Clang = llvm::make_unique<CompilerInstance>(PCHs);
   Clang->setInvocation(std::move(CI));
   Clang->createDiagnostics(&DiagsClient, false);