[Lldb-commits] [lldb] ca79145 - [lldb] Avoid FileSystem::Resolve for cached files in the SourceManager
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Tue Jun 27 14:19:07 PDT 2023
Author: Jonas Devlieghere
Date: 2023-06-27T14:19:02-07:00
New Revision: ca7914564e676fe52fa80049d9c6932653522424
URL: https://github.com/llvm/llvm-project/commit/ca7914564e676fe52fa80049d9c6932653522424
DIFF: https://github.com/llvm/llvm-project/commit/ca7914564e676fe52fa80049d9c6932653522424.diff
LOG: [lldb] Avoid FileSystem::Resolve for cached files in the SourceManager
Currently, source files are cached by their resolved path. This means
that before we can query the cache, we potentially have to resolve the
path, which can be slow. This patch avoids the call to FileSystem::Resolve
by caching both the resolved and unresolved path. We now only resolve
the path once when we create and cache a new file.
rdar://110787562
Differential revision: https://reviews.llvm.org/D153726
Added:
Modified:
lldb/include/lldb/Core/SourceManager.h
lldb/include/lldb/Host/FileSystem.h
lldb/source/Core/SourceManager.cpp
lldb/source/Host/common/FileSystem.cpp
lldb/unittests/Core/SourceManagerTest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Core/SourceManager.h b/lldb/include/lldb/Core/SourceManager.h
index 2ad6294e0f6cc..24a567747bda8 100644
--- a/lldb/include/lldb/Core/SourceManager.h
+++ b/lldb/include/lldb/Core/SourceManager.h
@@ -68,6 +68,9 @@ class SourceManager {
llvm::sys::TimePoint<> GetTimestamp() const { return m_mod_time; }
protected:
+ /// Set file and update modification time.
+ void SetFileSpec(FileSpec file_spec);
+
bool CalculateLineOffsets(uint32_t line = UINT32_MAX);
FileSpec m_file_spec_orig; // The original file spec that was used (can be
@@ -101,7 +104,7 @@ class SourceManager {
SourceFileCache() = default;
~SourceFileCache() = default;
- void AddSourceFile(const FileSP &file_sp);
+ void AddSourceFile(const FileSpec &file_spec, FileSP file_sp);
FileSP FindSourceFile(const FileSpec &file_spec) const;
// Removes all elements from the cache.
@@ -109,7 +112,9 @@ class SourceManager {
void Dump(Stream &stream) const;
- protected:
+ private:
+ void AddSourceFileImpl(const FileSpec &file_spec, FileSP file_sp);
+
typedef std::map<FileSpec, FileSP> FileCache;
FileCache m_file_cache;
};
diff --git a/lldb/include/lldb/Host/FileSystem.h b/lldb/include/lldb/Host/FileSystem.h
index e25fc9983c1f0..640f3846e448c 100644
--- a/lldb/include/lldb/Host/FileSystem.h
+++ b/lldb/include/lldb/Host/FileSystem.h
@@ -12,7 +12,9 @@
#include "lldb/Host/File.h"
#include "lldb/Utility/DataBuffer.h"
#include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/LLDBAssert.h"
#include "lldb/Utility/Status.h"
+#include "lldb/Utility/TildeExpressionResolver.h"
#include "llvm/Support/Chrono.h"
#include "llvm/Support/VirtualFileSystem.h"
@@ -30,17 +32,25 @@ class FileSystem {
static const char *DEV_NULL;
static const char *PATH_CONVERSION_ERROR;
- FileSystem() : m_fs(llvm::vfs::getRealFileSystem()) {}
+ FileSystem()
+ : m_fs(llvm::vfs::getRealFileSystem()),
+ m_tilde_resolver(std::make_unique<StandardTildeExpressionResolver>()) {}
FileSystem(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs)
- : m_fs(std::move(fs)) {}
+ : m_fs(std::move(fs)),
+ m_tilde_resolver(std::make_unique<StandardTildeExpressionResolver>()) {}
+ FileSystem(std::unique_ptr<TildeExpressionResolver> tilde_resolver)
+ : m_fs(llvm::vfs::getRealFileSystem()),
+ m_tilde_resolver(std::move(tilde_resolver)) {}
FileSystem(const FileSystem &fs) = delete;
FileSystem &operator=(const FileSystem &fs) = delete;
static FileSystem &Instance();
- static void Initialize();
- static void Initialize(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs);
+ template <class... T> static void Initialize(T &&...t) {
+ lldbassert(!InstanceImpl() && "Already initialized.");
+ InstanceImpl().emplace(std::forward<T>(t)...);
+ }
static void Terminate();
Status Symlink(const FileSpec &src, const FileSpec &dst);
@@ -197,6 +207,7 @@ class FileSystem {
private:
static std::optional<FileSystem> &InstanceImpl();
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> m_fs;
+ std::unique_ptr<TildeExpressionResolver> m_tilde_resolver;
std::string m_home_directory;
};
} // namespace lldb_private
diff --git a/lldb/source/Core/SourceManager.cpp b/lldb/source/Core/SourceManager.cpp
index 1cb119fb9397b..04f3065b290b4 100644
--- a/lldb/source/Core/SourceManager.cpp
+++ b/lldb/source/Core/SourceManager.cpp
@@ -75,13 +75,10 @@ SourceManager::FileSP SourceManager::GetFile(const FileSpec &file_spec) {
if (!file_spec)
return nullptr;
- FileSpec resolved_fspec = file_spec;
- resolve_tilde(resolved_fspec);
-
DebuggerSP debugger_sp(m_debugger_wp.lock());
FileSP file_sp;
if (debugger_sp && debugger_sp->GetUseSourceCache())
- file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(resolved_fspec);
+ file_sp = debugger_sp->GetSourceFileCache().FindSourceFile(file_spec);
TargetSP target_sp(m_target_wp.lock());
@@ -99,12 +96,12 @@ SourceManager::FileSP SourceManager::GetFile(const FileSpec &file_spec) {
// If file_sp is no good or it points to a non-existent file, reset it.
if (!file_sp || !FileSystem::Instance().Exists(file_sp->GetFileSpec())) {
if (target_sp)
- file_sp = std::make_shared<File>(resolved_fspec, target_sp.get());
+ file_sp = std::make_shared<File>(file_spec, target_sp.get());
else
- file_sp = std::make_shared<File>(resolved_fspec, debugger_sp);
+ file_sp = std::make_shared<File>(file_spec, debugger_sp);
if (debugger_sp && debugger_sp->GetUseSourceCache())
- debugger_sp->GetSourceFileCache().AddSourceFile(file_sp);
+ debugger_sp->GetSourceFileCache().AddSourceFile(file_spec, file_sp);
}
return file_sp;
}
@@ -393,15 +390,13 @@ void SourceManager::FindLinesMatchingRegex(FileSpec &file_spec,
SourceManager::File::File(const FileSpec &file_spec,
lldb::DebuggerSP debugger_sp)
- : m_file_spec_orig(file_spec), m_file_spec(file_spec),
- m_mod_time(FileSystem::Instance().GetModificationTime(file_spec)),
+ : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(),
m_debugger_wp(debugger_sp) {
CommonInitializer(file_spec, nullptr);
}
SourceManager::File::File(const FileSpec &file_spec, Target *target)
- : m_file_spec_orig(file_spec), m_file_spec(file_spec),
- m_mod_time(FileSystem::Instance().GetModificationTime(file_spec)),
+ : m_file_spec_orig(file_spec), m_file_spec(), m_mod_time(),
m_debugger_wp(target ? target->GetDebugger().shared_from_this()
: DebuggerSP()) {
CommonInitializer(file_spec, target);
@@ -409,13 +404,16 @@ SourceManager::File::File(const FileSpec &file_spec, Target *target)
void SourceManager::File::CommonInitializer(const FileSpec &file_spec,
Target *target) {
+ // Set the file and update the modification time.
+ SetFileSpec(file_spec);
+
+ // File doesn't exist.
if (m_mod_time == llvm::sys::TimePoint<>()) {
if (target) {
m_source_map_mod_id = target->GetSourcePathMap().GetModificationID();
+ // If this is just a file name, try finding it in the target.
if (!file_spec.GetDirectory() && file_spec.GetFilename()) {
- // If this is just a file name, lets see if we can find it in the
- // target:
bool check_inlines = false;
SymbolContextList sc_list;
size_t num_matches =
@@ -443,13 +441,12 @@ void SourceManager::File::CommonInitializer(const FileSpec &file_spec,
SymbolContext sc;
sc_list.GetContextAtIndex(0, sc);
if (sc.comp_unit)
- m_file_spec = sc.comp_unit->GetPrimaryFile();
- m_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
+ SetFileSpec(sc.comp_unit->GetPrimaryFile());
}
}
}
- resolve_tilde(m_file_spec);
- // Try remapping if m_file_spec does not correspond to an existing file.
+
+ // Try remapping the file if it doesn't exist.
if (!FileSystem::Instance().Exists(m_file_spec)) {
// Check target specific source remappings (i.e., the
// target.source-map setting), then fall back to the module
@@ -460,18 +457,23 @@ void SourceManager::File::CommonInitializer(const FileSpec &file_spec,
if (target->GetImages().FindSourceFile(m_file_spec, new_spec))
remapped = new_spec;
}
- if (remapped) {
- m_file_spec = *remapped;
- m_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
- }
+ if (remapped)
+ SetFileSpec(*remapped);
}
}
}
+ // If the file exists, read in the data.
if (m_mod_time != llvm::sys::TimePoint<>())
m_data_sp = FileSystem::Instance().CreateDataBuffer(m_file_spec);
}
+void SourceManager::File::SetFileSpec(FileSpec file_spec) {
+ resolve_tilde(file_spec);
+ m_file_spec = std::move(file_spec);
+ m_mod_time = FileSystem::Instance().GetModificationTime(m_file_spec);
+}
+
uint32_t SourceManager::File::GetLineOffset(uint32_t line) {
if (line == 0)
return UINT32_MAX;
@@ -708,12 +710,22 @@ bool SourceManager::File::GetLine(uint32_t line_no, std::string &buffer) {
return true;
}
-void SourceManager::SourceFileCache::AddSourceFile(const FileSP &file_sp) {
- FileSpec file_spec = file_sp->GetFileSpec();
+void SourceManager::SourceFileCache::AddSourceFile(const FileSpec &file_spec,
+ FileSP file_sp) {
+ assert(file_sp && "invalid FileSP");
+
+ AddSourceFileImpl(file_spec, file_sp);
+ const FileSpec &resolved_file_spec = file_sp->GetFileSpec();
+ if (file_spec != resolved_file_spec)
+ AddSourceFileImpl(file_sp->GetFileSpec(), file_sp);
+}
+
+void SourceManager::SourceFileCache::AddSourceFileImpl(
+ const FileSpec &file_spec, FileSP file_sp) {
FileCache::iterator pos = m_file_cache.find(file_spec);
- if (pos == m_file_cache.end())
+ if (pos == m_file_cache.end()) {
m_file_cache[file_spec] = file_sp;
- else {
+ } else {
if (file_sp != pos->second)
m_file_cache[file_spec] = file_sp;
}
diff --git a/lldb/source/Host/common/FileSystem.cpp b/lldb/source/Host/common/FileSystem.cpp
index 1fe4e8fdd500e..52227a9f63a52 100644
--- a/lldb/source/Host/common/FileSystem.cpp
+++ b/lldb/source/Host/common/FileSystem.cpp
@@ -9,8 +9,6 @@
#include "lldb/Host/FileSystem.h"
#include "lldb/Utility/DataBufferLLVM.h"
-#include "lldb/Utility/LLDBAssert.h"
-#include "lldb/Utility/TildeExpressionResolver.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/Errno.h"
@@ -46,16 +44,6 @@ using namespace llvm;
FileSystem &FileSystem::Instance() { return *InstanceImpl(); }
-void FileSystem::Initialize() {
- lldbassert(!InstanceImpl() && "Already initialized.");
- InstanceImpl().emplace();
-}
-
-void FileSystem::Initialize(IntrusiveRefCntPtr<vfs::FileSystem> fs) {
- lldbassert(!InstanceImpl() && "Already initialized.");
- InstanceImpl().emplace(fs);
-}
-
void FileSystem::Terminate() {
lldbassert(InstanceImpl() && "Already terminated.");
InstanceImpl().reset();
@@ -239,9 +227,9 @@ void FileSystem::Resolve(SmallVectorImpl<char> &path) {
// Resolve tilde in path.
SmallString<128> resolved(path.begin(), path.end());
- StandardTildeExpressionResolver Resolver;
- Resolver.ResolveFullPath(llvm::StringRef(path.begin(), path.size()),
- resolved);
+ assert(m_tilde_resolver && "must initialize tilde resolver in constructor");
+ m_tilde_resolver->ResolveFullPath(llvm::StringRef(path.begin(), path.size()),
+ resolved);
// Try making the path absolute if it exists.
SmallString<128> absolute(resolved.begin(), resolved.end());
diff --git a/lldb/unittests/Core/SourceManagerTest.cpp b/lldb/unittests/Core/SourceManagerTest.cpp
index 9dcd048ce3fb2..75c6baf485911 100644
--- a/lldb/unittests/Core/SourceManagerTest.cpp
+++ b/lldb/unittests/Core/SourceManagerTest.cpp
@@ -10,12 +10,17 @@
#include "lldb/Host/FileSystem.h"
#include "gtest/gtest.h"
+#include "TestingSupport/MockTildeExpressionResolver.h"
+
using namespace lldb;
using namespace lldb_private;
class SourceFileCache : public ::testing::Test {
public:
- void SetUp() override { FileSystem::Initialize(); }
+ void SetUp() override {
+ FileSystem::Initialize(std::unique_ptr<TildeExpressionResolver>(
+ new MockTildeExpressionResolver("Jonas", "/jonas")));
+ }
void TearDown() override { FileSystem::Terminate(); }
};
@@ -26,7 +31,7 @@ TEST_F(SourceFileCache, FindSourceFileFound) {
FileSpec foo_file_spec("foo");
auto foo_file_sp =
std::make_shared<SourceManager::File>(foo_file_spec, nullptr);
- cache.AddSourceFile(foo_file_sp);
+ cache.AddSourceFile(foo_file_spec, foo_file_sp);
// Query: foo, expect found.
FileSpec another_foo_file_spec("foo");
@@ -40,9 +45,32 @@ TEST_F(SourceFileCache, FindSourceFileNotFound) {
FileSpec foo_file_spec("foo");
auto foo_file_sp =
std::make_shared<SourceManager::File>(foo_file_spec, nullptr);
- cache.AddSourceFile(foo_file_sp);
+ cache.AddSourceFile(foo_file_spec, foo_file_sp);
// Query: bar, expect not found.
FileSpec bar_file_spec("bar");
ASSERT_EQ(cache.FindSourceFile(bar_file_spec), nullptr);
}
+
+TEST_F(SourceFileCache, FindSourceFileByUnresolvedPath) {
+ SourceManager::SourceFileCache cache;
+
+ FileSpec foo_file_spec("~/foo");
+
+ // Mimic the resolution in SourceManager::GetFile.
+ FileSpec resolved_foo_file_spec = foo_file_spec;
+ FileSystem::Instance().Resolve(resolved_foo_file_spec);
+
+ // Create the file with the resolved file spec.
+ auto foo_file_sp =
+ std::make_shared<SourceManager::File>(resolved_foo_file_spec, nullptr);
+
+ // Cache the result with the unresolved file spec.
+ cache.AddSourceFile(foo_file_spec, foo_file_sp);
+
+ // Query the unresolved path.
+ EXPECT_EQ(cache.FindSourceFile(FileSpec("~/foo")), foo_file_sp);
+
+ // Query the resolved path.
+ EXPECT_EQ(cache.FindSourceFile(FileSpec("/jonas/foo")), foo_file_sp);
+}
More information about the lldb-commits
mailing list