[PATCH] D89580: SourceManager: Fix an SLocEntry memory regression introduced with FileEntryRef

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

dexonsmith created this revision.
dexonsmith added reviewers: arphaman, Bigcheese.
Herald added a subscriber: ributzka.
dexonsmith requested review of this revision.

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.


https://reviews.llvm.org/D89580

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


Index: clang/lib/Basic/SourceManager.cpp
===================================================================
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -1748,7 +1748,7 @@
           // 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.
Index: clang/include/clang/Basic/SourceManager.h
===================================================================
--- clang/include/clang/Basic/SourceManager.h
+++ clang/include/clang/Basic/SourceManager.h
@@ -105,6 +105,8 @@
     ///
     /// 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.
@@ -113,6 +115,11 @@
     /// 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.  This is owned by the SourceManager
@@ -244,7 +251,11 @@
   /// 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;
@@ -269,10 +280,6 @@
     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, const ContentCache &Con,
@@ -283,7 +290,7 @@
       X.HasLineDirectives = false;
       X.ContentAndKind.setPointer(&Con);
       X.ContentAndKind.setInt(FileCharacter);
-      X.Filename = Filename;
+      const_cast<ContentCache &>(Con).Filename = Filename;
       return X;
     }
 
@@ -311,7 +318,7 @@
 
     /// 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
@@ -432,6 +439,13 @@
     }
   };
 
+  // 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89580.298700.patch
Type: text/x-patch
Size: 3966 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201016/3539c249/attachment-0001.bin>