Speed up preamble loading

[yvvan]

Summary

On Windows loading preamble and caching global completion takes a while.
This is the way to improve it.
Please comment if I misuse something.

[yvvan]

fix member variable name, add comments

[arphaman]

Please post the patch with full context git diff -U9999.

[yvvan]

diff with -U9999

[yvvan]

Remove global completion cache part because it's probably not always valid.

[arphaman]

Can you use a local map in TranslateStoredDiagnostics instead of storing one in the ASTUnit, or do you need to keep it around?

[yvvan]

In D33493#774264, @arphaman wrote:

Can you use a local map in TranslateStoredDiagnostics instead of storing one in the ASTUnit, or do you need to keep it around?

The whole purpose is to use it between different TranslateStoredDiagnostics calls because it scans the same files every time. And this cache allows it to reuse the source location.

[arphaman]

include/clang/Frontend/ASTUnit.h

192 ↗

(On Diff #101533)

You can use an llvm::StringMap instead.

lib/Frontend/ASTUnit.cpp

1152 ↗

(On Diff #101533)

Why is clear in an else here? We always create a new SourceManager in this function, so the previously cached locations will be invalid, so shouldn't we always clear the cache before TranslateStoredDiagnostics?

[yvvan]

include/clang/Frontend/ASTUnit.h

192 ↗

(On Diff #101533)

I will change that

lib/Frontend/ASTUnit.cpp

1152 ↗

(On Diff #101533)

When we load diagnostics that means that preamble has not changed. Doesn't that mean that source locations can be reused? What can cause them to become invalid?
When preamble is invalidated - cache is cleared.

I can keep cache only during TranslateStoredDiagnostics calls but in that case performance improvement is way less.

But if you say that current solution is invalid I will do that

[arphaman]

lib/Frontend/ASTUnit.cpp

1152 ↗

(On Diff #101533)

You're right actually, we can reuse them. We already do make that assumption that the preamble's source locations can be reused in checkAndSanitizeDiags. This code is fine then, sorry about the confusion.

You should mention in the comment for the SrcLocCache that we cache only the source locations from the preamble as we can guarantee that they will stay valid when the source manager is re-created.

[yvvan]

Improve commentary. Use StringMap

[arphaman]

LGTM with one inline request below.

I also have a question: what kind of performance benefits do you get for the preamble load times?

lib/Frontend/ASTUnit.cpp

2620 ↗

(On Diff #101865)

Please move the FileID FID declaration from above into here because we only use it in this if.

[yvvan]

"what kind of performance benefits do you get for the preamble load times?"
In cases where many windows headers are included preamble loading takes almost the half of reparse time. With this fix time to load preamble goes towards zero.

cfe/trunk/include/clang/Frontend/ASTUnit.h

private:
   /// If zero, we will never build a precompiled preamble. Otherwise,
   /// it's treated as a counter that decrements each time we reparse
   /// without the benefit of a precompiled preamble. When it hits 1,
   /// we'll attempt to rebuild the precompiled header. This way, if
   /// building the precompiled preamble fails, we won't try again for
   /// some number of calls.
   unsigned PreambleRebuildCounter;
 
+  /// \brief Cache pairs "filename - source location"
+  ///
+  /// Cache contains only source locations from preamble so it is
+  /// guaranteed that they stay valid when the SourceManager is recreated.
+  /// This cache is used when loading preambule to increase performance
+  /// of that loading. It must be cleared when preamble is recreated.
+  llvm::StringMap<SourceLocation> PreambleSrcLocCache;
+
 public:
   class PreambleData {
     const FileEntry *File;
     std::vector<char> Buffer;
     mutable unsigned NumLines;
     
   public:
     PreambleData() : File(nullptr), NumLines(0) { }

cfe/trunk/lib/Frontend/ASTUnit.cpp

llvm::CrashRecoveryContextCleanupRegistrar<TopLevelDeclTrackerAction>
     ActCleanup(Act.get());
 
   if (!Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0]))
     goto error;
 
   if (SavedMainFileBuffer)
     TranslateStoredDiagnostics(getFileManager(), getSourceManager(),
                                PreambleDiagnostics, StoredDiagnostics);
+  else
+    PreambleSrcLocCache.clear();
 
   if (!Act->Execute())
     goto error;
 
   transferASTDataFromCompilerInstance(*Clang);
   
   Act->EndSourceFile();
 
void ASTUnit::TranslateStoredDiagnostics(
                           FileManager &FileMgr,
                           SourceManager &SrcMgr,
                           const SmallVectorImpl<StandaloneDiagnostic> &Diags,
                           SmallVectorImpl<StoredDiagnostic> &Out) {
   // Map the standalone diagnostic into the new source manager. We also need to
   // remap all the locations to the new view. This includes the diag location,
   // any associated source ranges, and the source ranges of associated fix-its.
   // FIXME: There should be a cleaner way to do this.
-
   SmallVector<StoredDiagnostic, 4> Result;
   Result.reserve(Diags.size());
-  const FileEntry *PreviousFE = nullptr;
-  FileID FID;
+
   for (const StandaloneDiagnostic &SD : Diags) {
     // Rebuild the StoredDiagnostic.
     if (SD.Filename.empty())
       continue;
     const FileEntry *FE = FileMgr.getFile(SD.Filename);
     if (!FE)
       continue;
-    if (FE != PreviousFE) {
-      FID = SrcMgr.translateFile(FE);
-      PreviousFE = FE;
+    SourceLocation FileLoc;
+    auto ItFileID = PreambleSrcLocCache.find(SD.Filename);
+    if (ItFileID == PreambleSrcLocCache.end()) {
+      FileID FID = SrcMgr.translateFile(FE);
+      FileLoc = SrcMgr.getLocForStartOfFile(FID);
+      PreambleSrcLocCache[SD.Filename] = FileLoc;
+    } else {
+      FileLoc = ItFileID->getValue();
     }
-    SourceLocation FileLoc = SrcMgr.getLocForStartOfFile(FID);
+
     if (FileLoc.isInvalid())
       continue;
     SourceLocation L = FileLoc.getLocWithOffset(SD.LocOffset);
     FullSourceLoc Loc(L, SrcMgr);
 
     SmallVector<CharSourceRange, 4> Ranges;
     Ranges.reserve(SD.Ranges.size());
     for (const auto &Range : SD.Ranges) {