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
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
