Avoid need for SLocEntryLoaded BitVector #67960

ktf · 2023-10-02T08:36:15Z

The BitVector is currently used to keep track of which entries of LoadedSLocEntryTable have been loaded. Such information can be stored in the SLocEntry itself. Moreover, thanks to the fact that LoadedSLocEntryTable is now a PagedVector, we can simply consider elements of unmaterialised pages as not loaded.

This trades reducing the number of OffsetBits by one for reducing memory usage of the SourceManager by LoadedSLocEntryTable.size()/8 bytes.

The BitVector is currently used to keep track of which entries of LoadedSLocEntryTable have been loaded. Such information can be stored in the SLocEntry itself. Moreover, thanks to the fact that LoadedSLocEntryTable is now a PagedVector, we can simply consider elements of unmaterialised pages as not loaded. This trades reducing the number of OffsetBits by one for reducing memory usage of the SourceManager by LoadedSLocEntryTable.size()/8 bytes.

llvmbot · 2023-10-02T08:37:15Z

@llvm/pr-subscribers-llvm-adt

@llvm/pr-subscribers-clang

Changes

The BitVector is currently used to keep track of which entries of LoadedSLocEntryTable have been loaded. Such information can be stored in the SLocEntry itself. Moreover, thanks to the fact that LoadedSLocEntryTable is now a PagedVector, we can simply consider elements of unmaterialised pages as not loaded.

This trades reducing the number of OffsetBits by one for reducing memory usage of the SourceManager by LoadedSLocEntryTable.size()/8 bytes.

Full diff: https://github.com/llvm/llvm-project/pull/67960.diff

3 Files Affected:

(modified) clang/include/clang/Basic/SourceManager.h (+7-9)
(modified) clang/lib/Basic/SourceManager.cpp (+16-11)
(modified) llvm/include/llvm/ADT/PagedVector.h (+6)

diff --git a/clang/include/clang/Basic/SourceManager.h b/clang/include/clang/Basic/SourceManager.h
index c1b24eec2759c71..d450280303d6785 100644
--- a/clang/include/clang/Basic/SourceManager.h
+++ b/clang/include/clang/Basic/SourceManager.h
@@ -474,9 +474,10 @@ static_assert(sizeof(FileInfo) <= sizeof(ExpansionInfo),
 /// SourceManager keeps an array of these objects, and they are uniquely
 /// identified by the FileID datatype.
 class SLocEntry {
-  static constexpr int OffsetBits = 8 * sizeof(SourceLocation::UIntTy) - 1;
+  static constexpr int OffsetBits = 8 * sizeof(SourceLocation::UIntTy) - 2;
   SourceLocation::UIntTy Offset : OffsetBits;
   SourceLocation::UIntTy IsExpansion : 1;
+  SourceLocation::UIntTy Loaded : 1;
   union {
     FileInfo File;
     ExpansionInfo Expansion;
@@ -489,6 +490,8 @@ class SLocEntry {
 
   bool isExpansion() const { return IsExpansion; }
   bool isFile() const { return !isExpansion(); }
+  [[nodiscard]] bool isLoaded() const { return Loaded; }
+  void setLoaded(bool Value) { Loaded = Value; }
 
   const FileInfo &getFile() const {
     assert(isFile() && "Not a file SLocEntry!");
@@ -716,13 +719,7 @@ class SourceManager : public RefCountedBase<SourceManager> {
   /// The highest possible offset is 2^31-1 (2^63-1 for 64-bit source
   /// locations), so CurrentLoadedOffset starts at 2^31 (2^63 resp.).
   static const SourceLocation::UIntTy MaxLoadedOffset =
-      1ULL << (8 * sizeof(SourceLocation::UIntTy) - 1);
-
-  /// A bitmap that indicates whether the entries of LoadedSLocEntryTable
-  /// have already been loaded from the external source.
-  ///
-  /// Same indexing as LoadedSLocEntryTable.
-  llvm::BitVector SLocEntryLoaded;
+      1ULL << (8 * sizeof(SourceLocation::UIntTy) - 2);
 
   /// An external source for source location entries.
   ExternalSLocEntrySource *ExternalSLocEntries = nullptr;
@@ -1710,7 +1707,8 @@ class SourceManager : public RefCountedBase<SourceManager> {
   const SrcMgr::SLocEntry &getLoadedSLocEntry(unsigned Index,
                                               bool *Invalid = nullptr) const {
     assert(Index < LoadedSLocEntryTable.size() && "Invalid index");
-    if (SLocEntryLoaded[Index])
+    if (LoadedSLocEntryTable.isMaterialized(Index) &&
+        LoadedSLocEntryTable[Index].isLoaded())
       return LoadedSLocEntryTable[Index];
     return loadSLocEntry(Index, Invalid);
   }
diff --git a/clang/lib/Basic/SourceManager.cpp b/clang/lib/Basic/SourceManager.cpp
index 3066cc53dbfd878..20e3f1252ddf077 100644
--- a/clang/lib/Basic/SourceManager.cpp
+++ b/clang/lib/Basic/SourceManager.cpp
@@ -336,7 +336,6 @@ void SourceManager::clearIDTables() {
   MainFileID = FileID();
   LocalSLocEntryTable.clear();
   LoadedSLocEntryTable.clear();
-  SLocEntryLoaded.clear();
   LastLineNoFileIDQuery = FileID();
   LastLineNoContentCache = nullptr;
   LastFileIDLookup = FileID();
@@ -373,7 +372,8 @@ void SourceManager::initializeForReplay(const SourceManager &Old) {
 
   // Ensure all SLocEntries are loaded from the external source.
   for (unsigned I = 0, N = Old.LoadedSLocEntryTable.size(); I != N; ++I)
-    if (!Old.SLocEntryLoaded[I])
+    if (!Old.LoadedSLocEntryTable.isMaterialized(I) ||
+        !Old.LoadedSLocEntryTable[I].isLoaded())
       Old.loadSLocEntry(I, nullptr);
 
   // Inherit any content cache data from the old source manager.
@@ -430,12 +430,14 @@ ContentCache &SourceManager::createMemBufferContentCache(
 
 const SrcMgr::SLocEntry &SourceManager::loadSLocEntry(unsigned Index,
                                                       bool *Invalid) const {
-  assert(!SLocEntryLoaded[Index]);
+  assert(!LoadedSLocEntryTable.isMaterialized(Index) ||
+         !LoadedSLocEntryTable[Index].isLoaded());
   if (ExternalSLocEntries->ReadSLocEntry(-(static_cast<int>(Index) + 2))) {
     if (Invalid)
       *Invalid = true;
     // If the file of the SLocEntry changed we could still have loaded it.
-    if (!SLocEntryLoaded[Index]) {
+    if (!LoadedSLocEntryTable.isMaterialized(Index) ||
+        !LoadedSLocEntryTable[Index].isLoaded()) {
       // Try to recover; create a SLocEntry so the rest of clang can handle it.
       if (!FakeSLocEntryForRecovery)
         FakeSLocEntryForRecovery = std::make_unique<SLocEntry>(SLocEntry::get(
@@ -458,7 +460,6 @@ SourceManager::AllocateLoadedSLocEntries(unsigned NumSLocEntries,
     return std::make_pair(0, 0);
   }
   LoadedSLocEntryTable.resize(LoadedSLocEntryTable.size() + NumSLocEntries);
-  SLocEntryLoaded.resize(LoadedSLocEntryTable.size());
   CurrentLoadedOffset -= TotalSize;
   int ID = LoadedSLocEntryTable.size();
   return std::make_pair(-ID - 1, CurrentLoadedOffset);
@@ -604,10 +605,12 @@ FileID SourceManager::createFileIDImpl(ContentCache &File, StringRef Filename,
     assert(LoadedID != -1 && "Loading sentinel FileID");
     unsigned Index = unsigned(-LoadedID) - 2;
     assert(Index < LoadedSLocEntryTable.size() && "FileID out of range");
-    assert(!SLocEntryLoaded[Index] && "FileID already loaded");
+    assert((!LoadedSLocEntryTable.isMaterialized(Index) ||
+            !LoadedSLocEntryTable[Index].isLoaded()) &&
+           "FileID already loaded");
     LoadedSLocEntryTable[Index] = SLocEntry::get(
         LoadedOffset, FileInfo::get(IncludePos, File, FileCharacter, Filename));
-    SLocEntryLoaded[Index] = true;
+    LoadedSLocEntryTable[Index].setLoaded(true);
     return FileID::get(LoadedID);
   }
   unsigned FileSize = File.getSize();
@@ -665,9 +668,11 @@ SourceManager::createExpansionLocImpl(const ExpansionInfo &Info,
     assert(LoadedID != -1 && "Loading sentinel FileID");
     unsigned Index = unsigned(-LoadedID) - 2;
     assert(Index < LoadedSLocEntryTable.size() && "FileID out of range");
-    assert(!SLocEntryLoaded[Index] && "FileID already loaded");
+    assert((!LoadedSLocEntryTable.isMaterialized(Index) ||
+            !LoadedSLocEntryTable[Index].isLoaded()) &&
+           "FileID already loaded");
     LoadedSLocEntryTable[Index] = SLocEntry::get(LoadedOffset, Info);
-    SLocEntryLoaded[Index] = true;
+    LoadedSLocEntryTable[Index].setLoaded(true);
     return SourceLocation::getMacroLoc(LoadedOffset);
   }
   LocalSLocEntryTable.push_back(SLocEntry::get(NextLocalOffset, Info));
@@ -2223,7 +2228,8 @@ LLVM_DUMP_METHOD void SourceManager::dump() const {
   std::optional<SourceLocation::UIntTy> NextStart;
   for (unsigned Index = 0; Index != LoadedSLocEntryTable.size(); ++Index) {
     int ID = -(int)Index - 2;
-    if (SLocEntryLoaded[Index]) {
+    if (LoadedSLocEntryTable.isMaterialized(Index) &&
+        LoadedSLocEntryTable[Index].isLoaded()) {
       DumpSLocEntry(ID, LoadedSLocEntryTable[Index], NextStart);
       NextStart = LoadedSLocEntryTable[Index].getOffset();
     } else {
@@ -2346,7 +2352,6 @@ size_t SourceManager::getDataStructureSizes() const {
   size_t size = llvm::capacity_in_bytes(MemBufferInfos) +
                 llvm::capacity_in_bytes(LocalSLocEntryTable) +
                 llvm::capacity_in_bytes(LoadedSLocEntryTable) +
-                llvm::capacity_in_bytes(SLocEntryLoaded) +
                 llvm::capacity_in_bytes(FileInfos);
 
   if (OverriddenFilesInfo)
diff --git a/llvm/include/llvm/ADT/PagedVector.h b/llvm/include/llvm/ADT/PagedVector.h
index 667bece6d718385..dfae3a667360f3a 100644
--- a/llvm/include/llvm/ADT/PagedVector.h
+++ b/llvm/include/llvm/ADT/PagedVector.h
@@ -103,6 +103,12 @@ template <typename T, size_t PageSize = 1024 / sizeof(T)> class PagedVector {
   /// Return the size of the vector.
   [[nodiscard]] size_t size() const { return Size; }
 
+  [[nodiscard]] bool isMaterialized(size_t Index) const {
+    assert(Index < Size);
+    assert(Index / PageSize < PageToDataPtrs.size());
+    return PageToDataPtrs[Index / PageSize] != nullptr;
+  }
+
   /// Resize the vector. Notice that the constructor of the elements will not
   /// be invoked until an element of a given page is accessed, at which point
   /// all the elements of the page will be constructed.

ktf · 2023-10-02T08:43:27Z

@vgvassilev IIUC, this is what you were proposing in some comment in #66430.

llvm/include/llvm/ADT/PagedVector.h

Co-authored-by: Jakub Kuderski <[email protected]>

llvm/include/llvm/ADT/PagedVector.h

Co-authored-by: Jakub Kuderski <[email protected]>

llvm/include/llvm/ADT/PagedVector.h

vgvassilev

That's indeed what I was proposing. Do we observe some performance benefits from this PR on the downstream codebase?

clang/include/clang/Basic/SourceManager.h

ktf · 2023-10-02T20:32:55Z

IIRC I saw a ~0.3 MB reduction in memory usage in my usual test. I do not see any performance change, although that is not particularly the focus of my investigation here, so I cannot exclude it (hopefully for the better).

llvm/unittests/ADT/PagedVectorTest.cpp

kuhar

The ADT change looks good to me, I'm not familiar with the clang code though.

clang/include/clang/Basic/SourceManager.h

Co-authored-by: Vassil Vassilev <[email protected]>

vgvassilev · 2023-12-22T08:32:37Z

@ktf what is the fate of this PR?

ktf · 2023-12-22T11:22:26Z

I was carried away by the heavy ion run and other priorities. i can try to have a look again after Christmas. Feel free to close if you prefer. Ciao, Giulio

…

On Fri, Dec 22 2023 at 9:32 AM, Vassil Vassilev ***@***.***> wrote: @ktf <https://github.com/ktf> what is the fate of this PR? — Reply to this email directly, view it on GitHub <#67960 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACSMC6EIMDLKG2FJ4BMQLYKVAS7AVCNFSM6AAAAAA5PCLZ4OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRXGM4DSOJXGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" llvm:adt labels Oct 2, 2023

vgvassilev requested review from zygoloid, AaronBallman and ChuanqiXu9 October 2, 2023 10:42

kuhar reviewed Oct 2, 2023

View reviewed changes

llvm/include/llvm/ADT/PagedVector.h Outdated Show resolved Hide resolved

llvm/include/llvm/ADT/PagedVector.h Show resolved Hide resolved

ktf and others added 2 commits October 2, 2023 16:58

Update llvm/include/llvm/ADT/PagedVector.h

cb27b89

Co-authored-by: Jakub Kuderski <[email protected]>

Update llvm/include/llvm/ADT/PagedVector.h

578b13d

kuhar reviewed Oct 2, 2023

View reviewed changes

llvm/include/llvm/ADT/PagedVector.h Outdated Show resolved Hide resolved

Update llvm/include/llvm/ADT/PagedVector.h

9462c05

Co-authored-by: Jakub Kuderski <[email protected]>

vgvassilev reviewed Oct 2, 2023

View reviewed changes

llvm/include/llvm/ADT/PagedVector.h Show resolved Hide resolved

Add test

cd3e760

vgvassilev reviewed Oct 2, 2023

View reviewed changes

clang/include/clang/Basic/SourceManager.h Outdated Show resolved Hide resolved

Update clang/include/clang/Basic/SourceManager.h

07b4679

kuhar reviewed Oct 2, 2023

View reviewed changes

llvm/unittests/ADT/PagedVectorTest.cpp Outdated Show resolved Hide resolved

Use EXPECT_TRUE / EXPECT_FALSE

0770744

kuhar reviewed Oct 3, 2023

View reviewed changes

vgvassilev reviewed Oct 3, 2023

View reviewed changes

clang/include/clang/Basic/SourceManager.h Outdated Show resolved Hide resolved

clang/include/clang/Basic/SourceManager.h Outdated Show resolved Hide resolved

ktf and others added 2 commits October 3, 2023 16:40

Update clang/include/clang/Basic/SourceManager.h

9d14a34

Co-authored-by: Vassil Vassilev <[email protected]>

Update clang/include/clang/Basic/SourceManager.h

354653e

Co-authored-by: Vassil Vassilev <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid need for SLocEntryLoaded BitVector #67960

Avoid need for SLocEntryLoaded BitVector #67960

ktf commented Oct 2, 2023

llvmbot commented Oct 2, 2023 •

edited

Loading

ktf commented Oct 2, 2023

vgvassilev left a comment

ktf commented Oct 2, 2023

kuhar left a comment

vgvassilev commented Dec 22, 2023

ktf commented Dec 22, 2023 via email

Avoid need for SLocEntryLoaded BitVector #67960

Are you sure you want to change the base?

Avoid need for SLocEntryLoaded BitVector #67960

Conversation

ktf commented Oct 2, 2023

llvmbot commented Oct 2, 2023 • edited Loading

ktf commented Oct 2, 2023

vgvassilev left a comment

Choose a reason for hiding this comment

ktf commented Oct 2, 2023

kuhar left a comment

Choose a reason for hiding this comment

vgvassilev commented Dec 22, 2023

ktf commented Dec 22, 2023 via email

llvmbot commented Oct 2, 2023 •

edited

Loading