dexonsmith updated this revision to Diff 298530.
dexonsmith retitled this revision from "WIP: FileManager: Shrink FileEntryRef
to the size of a pointer" to "FileManager: Shrink FileEntryRef to the size of a
pointer".
dexonsmith added a comment.
Rebase on top of https://reviews.llvm.org/D89521 to further reduce noise. I
think this is ready to go.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89488/new/
https://reviews.llvm.org/D89488
Files:
clang/include/clang/Basic/FileManager.h
clang/lib/Basic/FileManager.cpp
clang/lib/Basic/SourceManager.cpp
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===================================================================
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -485,29 +485,34 @@
// Set up a virtual file with a different size than FakeStatCache uses.
const FileEntry *File = Manager.getVirtualFile("/tmp/test", /*Size=*/10, 0);
ASSERT_TRUE(File);
- FileEntryRef Ref("/tmp/test", *File);
- EXPECT_TRUE(Ref.isValid());
- EXPECT_EQ(Ref.getSize(), 10);
+ const FileEntry &FE = *File;
+ EXPECT_TRUE(FE.isValid());
+ EXPECT_EQ(FE.getSize(), 10);
// Calling a second time should not affect the UID or size.
- unsigned VirtualUID = Ref.getUID();
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
- EXPECT_EQ(Ref.getUID(), VirtualUID);
- EXPECT_EQ(Ref.getSize(), 10);
+ unsigned VirtualUID = FE.getUID();
+ EXPECT_EQ(
+ &FE,
+ &expectedToOptional(Manager.getFileRef("/tmp/test"))->getFileEntry());
+ EXPECT_EQ(FE.getUID(), VirtualUID);
+ EXPECT_EQ(FE.getSize(), 10);
// Bypass the file.
- llvm::Optional<FileEntryRef> BypassRef = Manager.getBypassFile(Ref);
+ llvm::Optional<FileEntryRef> BypassRef =
+ Manager.getBypassFile(File->getLastRef());
ASSERT_TRUE(BypassRef);
EXPECT_TRUE(BypassRef->isValid());
- EXPECT_EQ(BypassRef->getName(), Ref.getName());
+ EXPECT_EQ("/tmp/test", BypassRef->getName());
// Check that it's different in the right ways.
EXPECT_NE(&BypassRef->getFileEntry(), File);
EXPECT_NE(BypassRef->getUID(), VirtualUID);
- EXPECT_NE(BypassRef->getSize(), Ref.getSize());
+ EXPECT_NE(BypassRef->getSize(), FE.getSize());
// The virtual file should still be returned when searching.
- EXPECT_EQ(*expectedToOptional(Manager.getFileRef("/tmp/test")), Ref);
+ EXPECT_EQ(
+ &FE,
+ &expectedToOptional(Manager.getFileRef("/tmp/test"))->getFileEntry());
}
} // anonymous namespace
Index: clang/lib/Basic/SourceManager.cpp
===================================================================
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -707,7 +707,7 @@
SourceManager::bypassFileContentsOverride(const FileEntry &File) {
assert(isFileOverridden(&File));
llvm::Optional<FileEntryRef> BypassFile =
- FileMgr.getBypassFile(FileEntryRef(File.getName(), File));
+ FileMgr.getBypassFile(File.getLastRef());
// If the file can't be found in the FS, give up.
if (!BypassFile)
Index: clang/lib/Basic/FileManager.cpp
===================================================================
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -212,11 +212,11 @@
SeenFileInsertResult.first->second.getError());
// Construct and return and FileEntryRef, unless it's a redirect to another
// filename.
- SeenFileEntryOrRedirect Value = *SeenFileInsertResult.first->second;
+ SeenFileTableValue Value = *SeenFileInsertResult.first->second;
FileEntry *FE;
- if (LLVM_LIKELY(FE = Value.dyn_cast<FileEntry *>()))
- return FileEntryRef(SeenFileInsertResult.first->first(), *FE);
- return getFileRef(*Value.get<const StringRef *>(), openFile, CacheFailure);
+ if (LLVM_LIKELY(FE = Value.V.dyn_cast<FileEntry *>()))
+ return FileEntryRef(*SeenFileInsertResult.first);
+ return FileEntryRef(*Value.V.get<const SeenFileTableValue::MapEntryTy *>());
}
// We've not seen this before. Fill it in.
@@ -268,24 +268,24 @@
// This occurs when one dir is symlinked to another, for example.
FileEntry &UFE = UniqueRealFiles[Status.getUniqueID()];
- NamedFileEnt->second = &UFE;
-
- // If the name returned by getStatValue is different than Filename, re-intern
- // the name.
- if (Status.getName() != Filename) {
- auto &NewNamedFileEnt =
- *SeenFileEntries.insert({Status.getName(), &UFE}).first;
- assert((*NewNamedFileEnt.second).get<FileEntry *>() == &UFE &&
+ if (Status.getName() == Filename) {
+ // The name matches. Set the FileEntry.
+ NamedFileEnt->second = SeenFileTableValue(UFE);
+ } else {
+ // Name mismatch. We need a redirect. First grab the actual entry we want
+ // to return.
+ auto &Redirection =
+ *SeenFileEntries.insert({Status.getName(), SeenFileTableValue(UFE)})
+ .first;
+ assert(Redirection.second->V.get<FileEntry *>() == &UFE &&
"filename from getStatValue() refers to wrong file");
- InterndFileName = NewNamedFileEnt.first().data();
- // In addition to re-interning the name, construct a redirecting seen file
- // entry, that will point to the name the filesystem actually wants to use.
- StringRef *Redirect = new (CanonicalNameStorage) StringRef(InterndFileName);
- auto SeenFileInsertResultIt = SeenFileEntries.find(Filename);
- assert(SeenFileInsertResultIt != SeenFileEntries.end() &&
- "unexpected SeenFileEntries cache miss");
- SeenFileInsertResultIt->second = Redirect;
- NamedFileEnt = &*SeenFileInsertResultIt;
+
+ // Cache the redirection in the previously-inserted entry, still available
+ // in the tentative return value.
+ NamedFileEnt->second = SeenFileTableValue(Redirection);
+
+ // Fix the tentative return value.
+ NamedFileEnt = &Redirection;
}
if (UFE.isValid()) { // Already have an entry with this inode, return it.
@@ -306,13 +306,10 @@
// corresponding FileEntry.
// FIXME: The Name should be removed from FileEntry once all clients
// adopt FileEntryRef.
- UFE.Name = InterndFileName;
-
- return FileEntryRef(InterndFileName, UFE);
+ return *(UFE.LastRef = FileEntryRef(*NamedFileEnt));
}
// Otherwise, we don't have this file yet, add it.
- UFE.Name = InterndFileName;
UFE.Size = Status.getSize();
UFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
UFE.Dir = DirInfo;
@@ -329,7 +326,7 @@
// We should still fill the path even if we aren't opening the file.
fillRealPathName(&UFE, InterndFileName);
}
- return FileEntryRef(InterndFileName, UFE);
+ return *(UFE.LastRef = FileEntryRef(*NamedFileEnt));
}
const FileEntry *
@@ -341,12 +338,12 @@
auto &NamedFileEnt = *SeenFileEntries.insert(
{Filename, std::errc::no_such_file_or_directory}).first;
if (NamedFileEnt.second) {
- SeenFileEntryOrRedirect Value = *NamedFileEnt.second;
+ SeenFileTableValue Value = *NamedFileEnt.second;
FileEntry *FE;
- if (LLVM_LIKELY(FE = Value.dyn_cast<FileEntry *>()))
+ if (LLVM_LIKELY(FE = Value.V.dyn_cast<FileEntry *>()))
return FE;
- return getVirtualFile(*Value.get<const StringRef *>(), Size,
- ModificationTime);
+ return &FileEntryRef(*Value.V.get<const SeenFileTableValue::MapEntryTy *>())
+ .getFileEntry();
}
// We've not seen this before, or the file is cached as non-existent.
@@ -372,7 +369,7 @@
Status.getUser(), Status.getGroup(), Size,
Status.getType(), Status.getPermissions());
- NamedFileEnt.second = UFE;
+ NamedFileEnt.second = SeenFileTableValue(*UFE);
// If we had already opened this file, close it now so we don't
// leak the descriptor. We're not going to use the file
@@ -381,6 +378,9 @@
UFE->closeFile();
// If we already have an entry with this inode, return it.
+ //
+ // FIXME: Surely this should add a reference by the new name, and return
+ // it instead...
if (UFE->isValid())
return UFE;
@@ -390,10 +390,10 @@
} else {
VirtualFileEntries.push_back(std::make_unique<FileEntry>());
UFE = VirtualFileEntries.back().get();
- NamedFileEnt.second = UFE;
+ NamedFileEnt.second = SeenFileTableValue(*UFE);
}
- UFE->Name = InterndFileName;
+ UFE->LastRef = FileEntryRef(NamedFileEnt);
UFE->Size = Size;
UFE->ModTime = ModificationTime;
UFE->Dir = *DirInfo;
@@ -409,17 +409,30 @@
if (getStatValue(VF.getName(), Status, /*isFile=*/true, /*F=*/nullptr))
return None;
- // Fill it in from the stat.
+ if (!SeenBypassFileEntries)
+ SeenBypassFileEntries =
+ std::make_unique<llvm::StringMap<llvm::ErrorOr<SeenFileTableValue>>>();
+
+ // If we've already bypassed just use the existing one.
+ auto Insertion = SeenBypassFileEntries->insert(
+ {VF.getName(), std::errc::no_such_file_or_directory});
+ if (!Insertion.second)
+ return FileEntryRef(*Insertion.first);
+
+ // Fill in the new entry from the stat.
BypassFileEntries.push_back(std::make_unique<FileEntry>());
const FileEntry &VFE = VF.getFileEntry();
FileEntry &BFE = *BypassFileEntries.back();
- BFE.Name = VFE.getName();
+ Insertion.first->second = SeenFileTableValue(BFE);
+ BFE.LastRef = FileEntryRef(*Insertion.first);
BFE.Size = Status.getSize();
BFE.Dir = VFE.Dir;
BFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
BFE.UID = NextFileUID++;
BFE.IsValid = true;
- return FileEntryRef(VF.getName(), BFE);
+
+ // Save the entry in the bypass table and return.
+ return FileEntryRef(*Insertion.first);
}
bool FileManager::FixupRelativePath(SmallVectorImpl<char> &path) const {
@@ -534,13 +547,13 @@
UIDToFiles.resize(NextFileUID);
// Map file entries
- for (llvm::StringMap<llvm::ErrorOr<SeenFileEntryOrRedirect>,
+ for (llvm::StringMap<llvm::ErrorOr<SeenFileTableValue>,
llvm::BumpPtrAllocator>::const_iterator
FE = SeenFileEntries.begin(),
FEEnd = SeenFileEntries.end();
FE != FEEnd; ++FE)
- if (llvm::ErrorOr<SeenFileEntryOrRedirect> Entry = FE->getValue()) {
- if (const auto *FE = (*Entry).dyn_cast<FileEntry *>())
+ if (llvm::ErrorOr<SeenFileTableValue> Entry = FE->getValue()) {
+ if (const auto *FE = Entry->V.dyn_cast<FileEntry *>())
UIDToFiles[FE->getUID()] = FE;
}
Index: clang/include/clang/Basic/FileManager.h
===================================================================
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -73,16 +73,31 @@
class FileEntry;
+/// A reference to the file entry that is associated with a particular
+/// filename, or a reference to another FileEntryRef that should be used
+/// instead.
+///
+/// The reference to another filename is specifically useful for Redirecting
+/// VFSs that use external names. In that case, the \c FileEntryRef returned
+/// by the \c FileManager will have the external name, and not the name that
+/// was used to lookup the file.
+struct SeenFileTableValue {
+ using MapEntryTy = llvm::StringMapEntry<llvm::ErrorOr<SeenFileTableValue>>;
+ llvm::PointerUnion<FileEntry *, const MapEntryTy *> V;
+
+ SeenFileTableValue() = delete;
+ SeenFileTableValue(FileEntry &FE) : V(&FE) {}
+ SeenFileTableValue(MapEntryTy &ME) : V(&ME) {}
+};
+
/// A reference to a \c FileEntry that includes the name of the file as it was
/// accessed by the FileManager's client.
class FileEntryRef {
public:
- FileEntryRef() = delete;
- FileEntryRef(StringRef Name, const FileEntry &Entry)
- : Name(Name), Entry(&Entry) {}
-
- const StringRef getName() const { return Name; }
- const FileEntry &getFileEntry() const { return *Entry; }
+ const StringRef getName() const { return Entry->first(); }
+ const FileEntry &getFileEntry() const {
+ return *Entry->second->V.get<FileEntry *>();
+ }
inline bool isValid() const;
inline off_t getSize() const;
@@ -91,15 +106,24 @@
inline time_t getModificationTime() const;
friend bool operator==(const FileEntryRef &LHS, const FileEntryRef &RHS) {
- return LHS.Entry == RHS.Entry && LHS.Name == RHS.Name;
+ return LHS.Entry == RHS.Entry;
}
friend bool operator!=(const FileEntryRef &LHS, const FileEntryRef &RHS) {
return !(LHS == RHS);
}
private:
- StringRef Name;
- const FileEntry *Entry;
+ friend class FileManager;
+
+ FileEntryRef() = delete;
+ explicit FileEntryRef(const SeenFileTableValue::MapEntryTy &Entry)
+ : Entry(&Entry) {
+ assert(Entry.second && "Expected payload");
+ assert(Entry.second->V && "Expected non-null");
+ assert(Entry.second->V.is<FileEntry *>() && "Expected FileEntry");
+ }
+
+ const SeenFileTableValue::MapEntryTy *Entry;
};
/// Cached information about one file (either on disk
@@ -110,7 +134,6 @@
class FileEntry {
friend class FileManager;
- StringRef Name; // Name of the file.
std::string RealPathName; // Real path to the file; could be empty.
off_t Size; // File size in bytes.
time_t ModTime; // Modification time of file.
@@ -123,6 +146,14 @@
/// The open file, if it is owned by the \p FileEntry.
mutable std::unique_ptr<llvm::vfs::File> File;
+ // First access name for this FileEntry.
+ //
+ // This is Optional only to allow delayed construction (FileEntryRef has no
+ // default constructor). It should always have a value in practice.
+ //
+ // TODO: remote this once everyone that needs a name uses FileEntryRef.
+ Optional<FileEntryRef> LastRef;
+
public:
FileEntry()
: UniqueID(0, 0), IsNamedPipe(false), IsValid(false)
@@ -131,7 +162,9 @@
FileEntry(const FileEntry &) = delete;
FileEntry &operator=(const FileEntry &) = delete;
- StringRef getName() const { return Name; }
+ StringRef getName() const { return LastRef->getName(); }
+ FileEntryRef getLastRef() const { return *LastRef; }
+
StringRef tryGetRealPathName() const { return RealPathName; }
bool isValid() const { return IsValid; }
off_t getSize() const { return Size; }
@@ -228,10 +261,16 @@
/// the file.
///
/// \see SeenDirEntries
- llvm::StringMap<llvm::ErrorOr<SeenFileEntryOrRedirect>,
- llvm::BumpPtrAllocator>
+ llvm::StringMap<llvm::ErrorOr<SeenFileTableValue>, llvm::BumpPtrAllocator>
SeenFileEntries;
+ /// A mirror of SeenFileEntries to give fake answers for getBypassFile().
+ ///
+ /// Don't bother hooking up a BumpPtrAllocator. This should be rarely used,
+ /// and only on error paths.
+ std::unique_ptr<llvm::StringMap<llvm::ErrorOr<SeenFileTableValue>>>
+ SeenBypassFileEntries;
+
/// The canonical names of files and directories .
llvm::DenseMap<const void *, llvm::StringRef> CanonicalNames;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
