[clang] 22e6b18 - SourceManager: Fix an SLocEntry memory regression introduced with FileEntryRef

[Duncan P. N. Exon Smith via cfe-commits]

Author: Duncan P. N. Exon Smith
Date: 2020-10-26T15:38:13-04:00
New Revision: 22e6b1863e74136908842d71b4f942313d89b273

URL: https://github.com/llvm/llvm-project/commit/22e6b1863e74136908842d71b4f942313d89b273
DIFF: https://github.com/llvm/llvm-project/commit/22e6b1863e74136908842d71b4f942313d89b273.diff

LOG: SourceManager: Fix an SLocEntry memory regression introduced with FileEntryRef

4dc5573acc0d2e7c59d8bac2543eb25cb4b32984 added `FileEntryRef` in order to
help enable sharing of a `FileManager` between `CompilerInstance`s.

It also added a `StringRef` with the filename on `FileInfo`. This
doubled `sizeof(FileInfo)`, bloating `sizeof(SLocEntry)`, of which we
have one for each (loaded and unloaded) file and macro expansion. This
causes a memory regression in modules builds.

Move the filename down into the `ContentCache`, which is a side data
structure for `FileInfo` that does not impact `sizeof(SLocEntry)`. Once
`FileEntryRef` is used for `ContentCache::OrigEntry` this can go away.

Differential Revision: https://reviews.llvm.org/D89580
Radar-Id: rdar://59908826

Added: 
    

Modified: 
    clang/include/clang/Basic/SourceManager.h
    clang/lib/Basic/SourceManager.cpp

Removed: 
    


################################################################################
diff  --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h
index 99428e6d7efa..ec9b2b7d0cfd 100644
--- a/clang/include/clang/Basic/SourceManager.h
+++ b/clang/include/clang/Basic/SourceManager.h
@@ -134,6 +134,8 @@ namespace SrcMgr {
     ///
     /// It is possible for this to be NULL if the ContentCache encapsulates
     /// an imaginary text buffer.
+    ///
+    /// FIXME: Turn this into a FileEntryRef and remove Filename.
     const FileEntry *OrigEntry;
 
     /// References the file which the contents were actually loaded from.
@@ -142,6 +144,11 @@ namespace SrcMgr {
     /// with the contents of another file.
     const FileEntry *ContentsEntry;
 
+    /// The filename that is used to access OrigEntry.
+    ///
+    /// FIXME: Remove this once OrigEntry is a FileEntryRef with a stable name.
+    StringRef Filename;
+
     /// A bump pointer allocated array of offsets for each source line.
     ///
     /// This is lazily computed.  The lines are owned by the SourceManager
@@ -266,7 +273,11 @@ namespace SrcMgr {
   /// from. This information encodes the \#include chain that a token was
   /// expanded from. The main include file has an invalid IncludeLoc.
   ///
-  /// FileInfos contain a "ContentCache *", with the contents of the file.
+  /// FileInfo should not grow larger than ExpansionInfo. Doing so will
+  /// cause memory to bloat in compilations with many unloaded macro
+  /// expansions, since the two data structurs are stored in a union in
+  /// SLocEntry. Extra fields should instead go in "ContentCache *", which
+  /// stores file contents and other bits on the side.
   ///
   class FileInfo {
     friend class clang::SourceManager;
@@ -291,10 +302,6 @@ namespace SrcMgr {
     llvm::PointerIntPair<const ContentCache*, 3, CharacteristicKind>
         ContentAndKind;
 
-    /// The filename that is used to access the file entry represented by the
-    /// content cache.
-    StringRef Filename;
-
   public:
     /// Return a FileInfo object.
     static FileInfo get(SourceLocation IL, ContentCache &Con,
@@ -305,7 +312,7 @@ namespace SrcMgr {
       X.HasLineDirectives = false;
       X.ContentAndKind.setPointer(&Con);
       X.ContentAndKind.setInt(FileCharacter);
-      X.Filename = Filename;
+      Con.Filename = Filename;
       return X;
     }
 
@@ -333,7 +340,7 @@ namespace SrcMgr {
 
     /// Returns the name of the file that was used when the file was loaded from
     /// the underlying file system.
-    StringRef getName() const { return Filename; }
+    StringRef getName() const { return getContentCache().Filename; }
   };
 
   /// Each ExpansionInfo encodes the expansion location - where
@@ -454,6 +461,13 @@ namespace SrcMgr {
     }
   };
 
+  // Assert that the \c FileInfo objects are no bigger than \c ExpansionInfo
+  // objects. This controls the size of \c SLocEntry, of which we have one for
+  // each macro expansion. The number of (unloaded) macro expansions can be
+  // very large. Any other fields needed in FileInfo should go in ContentCache.
+  static_assert(sizeof(FileInfo) <= sizeof(ExpansionInfo),
+                "FileInfo must be no larger than ExpansionInfo.");
+
   /// This is a discriminated union of FileInfo and ExpansionInfo.
   ///
   /// SourceManager keeps an array of these objects, and they are uniquely

diff  --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index 2ceb8046098f..6eae0f06a122 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -1734,7 +1734,7 @@ void SourceManager::computeMacroArgsCache(MacroArgsMap &MacroArgsCache,
           // Predefined header doesn't have a valid include location in main
           // file, but any files created by it should still be skipped when
           // computing macro args expanded in the main file.
-          (FID == MainFileID && Entry.getFile().Filename == "<built-in>");
+          (FID == MainFileID && Entry.getFile().getName() == "<built-in>");
       if (IncludedInFID) {
         // Skip the files/macros of the #include'd file, we only care about
         // macros that lexed macro arguments from our file.