JDevlieghere created this revision.
JDevlieghere added reviewers: bulbazord, clayborg, jingham, jasonmolenda.
Herald added a project: All.
JDevlieghere requested review of this revision.
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.
https://reviews.llvm.org/D153726
Files:
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
Index: lldb/unittests/Core/SourceManagerTest.cpp
===================================================================
--- lldb/unittests/Core/SourceManagerTest.cpp
+++ 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 @@
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 @@
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);
+}
Index: lldb/source/Host/common/FileSystem.cpp
===================================================================
--- lldb/source/Host/common/FileSystem.cpp
+++ 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 @@
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();
@@ -240,8 +228,8 @@
// Resolve tilde in path.
SmallString<128> resolved(path.begin(), path.end());
StandardTildeExpressionResolver Resolver;
- Resolver.ResolveFullPath(llvm::StringRef(path.begin(), path.size()),
- resolved);
+ 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());
Index: lldb/source/Core/SourceManager.cpp
===================================================================
--- lldb/source/Core/SourceManager.cpp
+++ lldb/source/Core/SourceManager.cpp
@@ -75,13 +75,10 @@
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());
@@ -98,13 +95,15 @@
// 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())) {
+ FileSpec resolved_fspec = file_spec;
+ resolve_tilde(resolved_fspec);
if (target_sp)
file_sp = std::make_shared<File>(resolved_fspec, target_sp.get());
else
file_sp = std::make_shared<File>(resolved_fspec, 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;
}
@@ -708,12 +707,22 @@
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;
}
Index: lldb/include/lldb/Host/FileSystem.h
===================================================================
--- lldb/include/lldb/Host/FileSystem.h
+++ 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 @@
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 @@
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
Index: lldb/include/lldb/Core/SourceManager.h
===================================================================
--- lldb/include/lldb/Core/SourceManager.h
+++ lldb/include/lldb/Core/SourceManager.h
@@ -101,7 +101,7 @@
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 +109,9 @@
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;
};
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
