[lldb] Avoid FileSystem::Resolve for cached files in the SourceManager

[JDevlieghere]

Summary

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.

[bulbazord]

lldb/source/Host/common/FileSystem.cpp

229–230

nit: Unused Resolver.

[JDevlieghere]

Remove unused Resolver.

[bulbazord]

LGTM!

lldb/include/lldb/Core/SourceManager.h

public:
 
     uint32_t GetLineLength(uint32_t line, bool include_newline_chars);
 
     uint32_t GetNumLines();
 
     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
                                // different from m_file_spec)
     FileSpec m_file_spec; // The actually file spec being used (if the target
                           // has source mappings, this might be different from
                           // m_file_spec_orig)
 
public:
   // The SourceFileCache class separates the source manager from the cache of
   // source files, so the cache can be stored in the Debugger, but the source
   // managers can be per target.
   class SourceFileCache {
   public:
     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.
     void Clear() { m_file_cache.clear(); }
 
     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;
   };
 
   // Constructors and Destructors
   // A source manager can be made with a non-null target, in which case it can
   // use the path remappings to find
   // source files that are not in their build locations.  With no target it

lldb/include/lldb/Host/FileSystem.h

 //===-- FileSystem.h --------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_HOST_FILESYSTEM_H
 #define LLDB_HOST_FILESYSTEM_H
 
 #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"
 
 #include "lldb/lldb-types.h"
 
 #include <cstdint>
 #include <cstdio>
 #include <optional>
 #include <sys/stat.h>
 
 namespace lldb_private {
 class FileSystem {
 public:
   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);
   Status Readlink(const FileSpec &src, FileSpec &dst);
 
   Status ResolveSymbolicLink(const FileSpec &src, FileSpec &dst);
 
   /// Wraps ::fopen in a platform-independent way.
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> GetVirtualFileSystem() {
     return m_fs;
   }
 
   void SetHomeDirectory(std::string home_directory);
 
 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
 
 #endif

lldb/source/Core/SourceManager.cpp

 
 // Destructor
 SourceManager::~SourceManager() = default;
 
 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());
 
   // It the target source path map has been updated, get this file again so we
   // can successfully remap the source file
   if (target_sp && file_sp &&
       file_sp->GetSourceMapModificationID() !=
           target_sp->GetSourcePathMap().GetModificationID())
     file_sp.reset();
 
   // Update the file contents if needed if we found a file
   if (file_sp)
     file_sp->UpdateIfNeeded();
 
   // 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;
 }
 
 static bool should_highlight_source(DebuggerSP debugger_sp) {
   if (!debugger_sp)
     return false;
 
void SourceManager::FindLinesMatchingRegex(FileSpec &file_spec,
   if (!file_sp)
     return;
   return file_sp->FindLinesMatchingRegex(regex, start_line, end_line,
                                          match_lines);
 }
 
 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);
 }
 
 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 =
             target->GetImages().ResolveSymbolContextForFilePath(
                 file_spec.GetFilename().AsCString(), 0, check_inlines,
                 SymbolContextItem(eSymbolContextModule |
                                   eSymbolContextCompUnit),
                 sc_list);
if (target) {
                   test_cu = sc.comp_unit;
               }
             }
           }
           if (!got_multiple) {
             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
         // specific remapping (i.e., the .dSYM remapping dictionary).
         auto remapped = target->GetSourcePathMap().FindFile(m_file_spec);
         if (!remapped) {
           FileSpec new_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;
 
   if (line == 1)
     return 0;
 
   if (CalculateLineOffsets(line)) {
if (end_offset == UINT32_MAX) {
     end_offset = m_data_sp->GetByteSize();
   }
   buffer.assign((const char *)m_data_sp->GetBytes() + start_offset,
                 end_offset - start_offset);
 
   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;
   }
 }
 
 SourceManager::FileSP SourceManager::SourceFileCache::FindSourceFile(
     const FileSpec &file_spec) const {
   FileSP file_sp;

lldb/source/Host/common/FileSystem.cpp

 //===-- FileSystem.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Threading.h"

 #include <vector>
 
 using namespace lldb;
 using namespace lldb_private;
 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();
 }
 
 std::optional<FileSystem> &FileSystem::InstanceImpl() {
   static std::optional<FileSystem> g_fs;
   return g_fs;
std::error_code FileSystem::GetRealPath(const Twine &path,
   return m_fs->getRealPath(path, output);
 }
 
 void FileSystem::Resolve(SmallVectorImpl<char> &path) {
   if (path.empty())
     return;
 
   // Resolve tilde in path.
   SmallString<128> resolved(path.begin(), path.end());
-  StandardTildeExpressionResolver Resolver;
-  Resolver.ResolveFullPath(llvm::StringRef(path.begin(), path.size()),
+  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());
   MakeAbsolute(absolute);
 
   path.clear();
   if (Exists(absolute)) {
     path.append(absolute.begin(), absolute.end());

lldb/unittests/Core/SourceManagerTest.cpp

 //===-- SourceManagerTest.cpp ---------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/SourceManager.h"
 #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(); }
 };
 
 TEST_F(SourceFileCache, FindSourceFileFound) {
   SourceManager::SourceFileCache cache;
 
   // Insert: foo
   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");
   ASSERT_EQ(cache.FindSourceFile(another_foo_file_spec), foo_file_sp);
 }
 
 TEST_F(SourceFileCache, FindSourceFileNotFound) {
   SourceManager::SourceFileCache cache;
 
   // Insert: foo
   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);
+}