[SharedCache] Avoid copying strings on each call to VM::MappingAtAddress

`PageMapping` was storing the path to the file it points within. This
was causing unnecessary work within `VM::MappingAtAddress` as the
`PageMapping`, and thus the path, is copied into the return value. This
copying was more expensive than the map lookup.

The file path is now stored within a `LazyMappedFileAccessor` class that
wraps the `SelfAllocatingWeakPtr`. This means the path is still
available via the `PageMapping`, but it does not need to be copied as
often.

This includes two additional improvements / optimizations while I was
touching the code in question:
1. `MMappedFileAccessor::Open` no longer performs two hash lookups when
   a file accessor already exists.
2. `VM::MapPages` takes the path by const reference to avoid an
   unnecessary copy.

3 files changed, 71 insertions, 56 deletions

diff --git a/view/sharedcache/core/SharedCache.cpp b/view/sharedcache/core/SharedCache.cpp
index 33a4654d..0eb52c6a 100644
--- a/view/sharedcache/core/SharedCache.cpp
+++ b/view/sharedcache/core/SharedCache.cpp
@@ -780,7 +780,7 @@ void SharedCache::PerformInitialLoad()
 				if (imageHeader->linkeditPresent && vm->AddressIsMapped(imageHeader->linkeditSegment.vmaddr))
 				{
 					auto mapping = vm->MappingAtAddress(imageHeader->linkeditSegment.vmaddr);
-					imageHeader->exportTriePath = mapping.first.filePath;
+					imageHeader->exportTriePath = mapping.first.fileAccessor->filePath();
 				}
 				MutableState().headers[start.second] = imageHeader.value();
 				CacheImage image;
@@ -3330,7 +3330,8 @@ extern "C"
 						images[i].mappings[j].vmAddress = sectionStart;
 						images[i].mappings[j].size = header.sections[j].size;
 						images[i].mappings[j].name = BNAllocString(header.sectionNames[j].c_str());
-						images[i].mappings[j].filePath = BNAllocString(vm->MappingAtAddress(sectionStart).first.filePath.c_str());
+						auto fileAccessor = vm->MappingAtAddress(sectionStart).first.fileAccessor;
+						images[i].mappings[j].filePath = BNAllocStringWithLength(fileAccessor->filePath().data(), fileAccessor->filePath().length());
 						images[i].mappings[j].loaded = cache->object->IsMemoryMapped(sectionStart);
 					}
 					i++;
diff --git a/view/sharedcache/core/VM.cpp b/view/sharedcache/core/VM.cpp
index 5aca51d9..72d6d202 100644
--- a/view/sharedcache/core/VM.cpp
+++ b/view/sharedcache/core/VM.cpp
@@ -203,55 +203,57 @@ void MMAP::Unmap()
 }
 
 
-std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>> MMappedFileAccessor::Open(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView, const uint64_t sessionID, const std::string &path, std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAllocationRoutine)
+std::shared_ptr<LazyMappedFileAccessor> MMappedFileAccessor::Open(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView, const uint64_t sessionID, const std::string &path, std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAllocationRoutine)
 {
 	std::scoped_lock<std::mutex> lock(fileAccessorsMutex);
-	if (fileAccessors.count(path) == 0)
-	{
-		auto fileAcccessor = std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>>(new SelfAllocatingWeakPtr<MMappedFileAccessor>(
-			// Allocator logic for the SelfAllocatingWeakPtr
-			[path=path, sessionID=sessionID, dscView](){
-				std::unique_lock<std::mutex> _lock(fileAccessorDequeMutex);
+	if (auto it = fileAccessors.find(path); it != fileAccessors.end()) {
+		return it->second;
+	}
 
-				// Iterate through held references and start removing them until we can get a file pointer
-				// FIXME: This could clear all currently used file pointers and still not get one. FIX!
-				// 		We should probably use a condition variable here to wait for a file pointer to be released!!!
-				for (auto& [_, fileAccessorDeque] : fileAccessorReferenceHolder)
-				{
-					if (fileAccessorSemaphore.try_acquire())
-						break;
-					fileAccessorDeque.pop_front();
-				}
+	auto fileAcccessor = std::make_shared<LazyMappedFileAccessor>(
+		path,
+		// Allocator logic for the SelfAllocatingWeakPtr
+		[path=path, sessionID=sessionID, dscView](){
+			std::unique_lock<std::mutex> _lock(fileAccessorDequeMutex);
 
-				mmapCount++;
-				_lock.unlock();
-				auto accessor = std::shared_ptr<MMappedFileAccessor>(new MMappedFileAccessor(ResolveFilePath(dscView, path)), [](MMappedFileAccessor* accessor){
-					// worker thread or we can deadlock on exit here.
-					BinaryNinja::WorkerEnqueue([accessor](){
-						fileAccessorSemaphore.release();
-						mmapCount--;
-						if (fileAccessors.count(accessor->m_path))
-						{
-							std::scoped_lock<std::mutex> lock(fileAccessorsMutex);
-							fileAccessors.erase(accessor->m_path);
-						}
-						delete accessor;
-					}, "MMappedFileAccessor Destructor");
-				});
-				_lock.lock();
-				// If some background thread has managed to try and open a file when the BV was already closed,
-				// 		we can still give them the file they want so they dont crash, but as soon as they let go it's gone.
-				if (!blockedSessionIDs.count(sessionID))
-					fileAccessorReferenceHolder[sessionID].push_back(accessor);
-				return accessor;
-			},
-			[postAllocationRoutine=postAllocationRoutine](std::shared_ptr<MMappedFileAccessor> accessor){
-				if (postAllocationRoutine)
-					postAllocationRoutine(accessor);
-			}));
-		fileAccessors.insert_or_assign(path, fileAcccessor);
-	}
-	return fileAccessors.at(path);
+			// Iterate through held references and start removing them until we can get a file pointer
+			// FIXME: This could clear all currently used file pointers and still not get one. FIX!
+			// 		We should probably use a condition variable here to wait for a file pointer to be released!!!
+			for (auto& [_, fileAccessorDeque] : fileAccessorReferenceHolder)
+			{
+				if (fileAccessorSemaphore.try_acquire())
+					break;
+				fileAccessorDeque.pop_front();
+			}
+
+			mmapCount++;
+			_lock.unlock();
+			auto accessor = std::shared_ptr<MMappedFileAccessor>(new MMappedFileAccessor(ResolveFilePath(dscView, path)), [](MMappedFileAccessor* accessor){
+				// worker thread or we can deadlock on exit here.
+				BinaryNinja::WorkerEnqueue([accessor](){
+					fileAccessorSemaphore.release();
+					mmapCount--;
+					if (fileAccessors.count(accessor->m_path))
+					{
+						std::scoped_lock<std::mutex> lock(fileAccessorsMutex);
+						fileAccessors.erase(accessor->m_path);
+					}
+					delete accessor;
+				}, "MMappedFileAccessor Destructor");
+			});
+			_lock.lock();
+			// If some background thread has managed to try and open a file when the BV was already closed,
+			// 		we can still give them the file they want so they dont crash, but as soon as they let go it's gone.
+			if (!blockedSessionIDs.count(sessionID))
+				fileAccessorReferenceHolder[sessionID].push_back(accessor);
+			return accessor;
+		},
+		[postAllocationRoutine=postAllocationRoutine](std::shared_ptr<MMappedFileAccessor> accessor){
+			if (postAllocationRoutine)
+				postAllocationRoutine(std::move(accessor));
+		});
+	fileAccessors.insert_or_assign(path, fileAcccessor);
+	return fileAcccessor;
 }
 
 
@@ -482,7 +484,7 @@ VM::~VM()
 }
 
 
-void VM::MapPages(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView, uint64_t sessionID, size_t vm_address, size_t fileoff, size_t size, std::string filePath, std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAllocationRoutine)
+void VM::MapPages(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView, uint64_t sessionID, size_t vm_address, size_t fileoff, size_t size, const std::string& filePath, std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAllocationRoutine)
 {
 	// The mappings provided for shared caches will always be page aligned.
 	// We can use this to our advantage and gain considerable performance via page tables.
@@ -495,7 +497,7 @@ void VM::MapPages(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView, uint64_t se
 	}
 
 	auto accessor = MMappedFileAccessor::Open(std::move(dscView), sessionID, filePath, postAllocationRoutine);
-	auto [it, inserted] = m_map.insert_or_assign({vm_address, vm_address + size}, PageMapping(std::move(filePath), std::move(accessor), fileoff));
+	auto [it, inserted] = m_map.insert_or_assign({vm_address, vm_address + size}, PageMapping(std::move(accessor), fileoff));
 	if (m_safe && !inserted)
 	{
 		BNLogWarn("Remapping page 0x%zx (f: 0x%zx)", vm_address, fileoff);
diff --git a/view/sharedcache/core/VM.h b/view/sharedcache/core/VM.h
index e47cf15e..955dcbec 100644
--- a/view/sharedcache/core/VM.h
+++ b/view/sharedcache/core/VM.h
@@ -104,12 +104,25 @@ class MMAP {
     void Unmap();
 };
 
+class LazyMappedFileAccessor : public SelfAllocatingWeakPtr<MMappedFileAccessor> {
+public:
+    LazyMappedFileAccessor(std::string filePath, std::function<std::shared_ptr<MMappedFileAccessor>()> allocator,
+            std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAlloc)
+        : SelfAllocatingWeakPtr(std::move(allocator), std::move(postAlloc)), m_filePath(std::move(filePath)) {
+    }
+
+    std::string_view filePath() const { return m_filePath; }
+
+private:
+    std::string m_filePath;
+};
+
 static uint64_t maxFPLimit;
 static std::mutex fileAccessorDequeMutex;
 static std::unordered_map<uint64_t, std::deque<std::shared_ptr<MMappedFileAccessor>>> fileAccessorReferenceHolder;
 static std::set<uint64_t> blockedSessionIDs;
 static std::mutex fileAccessorsMutex;
-static std::unordered_map<std::string, std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>>> fileAccessors;
+static std::unordered_map<std::string, std::shared_ptr<LazyMappedFileAccessor>> fileAccessors;
 static counting_semaphore fileAccessorSemaphore(0);
 
 static std::atomic<uint64_t> mmapCount = 0;
@@ -123,7 +136,7 @@ public:
 	MMappedFileAccessor(const std::string &path);
 	~MMappedFileAccessor();
 
-	static std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>> Open(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView, const uint64_t sessionID, const std::string &path, std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAllocationRoutine = nullptr);
+	static std::shared_ptr<LazyMappedFileAccessor> Open(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView, const uint64_t sessionID, const std::string &path, std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAllocationRoutine = nullptr);
 
 	static void CloseAll(const uint64_t sessionID);
 
@@ -179,11 +192,10 @@ public:
 
 
 struct PageMapping {
-    std::string filePath;
-	std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>> fileAccessor;
+    std::shared_ptr<LazyMappedFileAccessor> fileAccessor;
     size_t fileOffset;
-	PageMapping(std::string filePath, std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>> fileAccessor, size_t fileOffset)
-		: filePath(std::move(filePath)), fileAccessor(std::move(fileAccessor)), fileOffset(fileOffset) {}
+    PageMapping(std::shared_ptr<LazyMappedFileAccessor> fileAccessor, size_t fileOffset)
+        : fileAccessor(std::move(fileAccessor)), fileOffset(fileOffset) {}
 };
 
 
@@ -249,7 +261,7 @@ public:
 
     ~VM();
 
-    void MapPages(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView, uint64_t sessionID, size_t vm_address, size_t fileoff, size_t size, std::string filePath, std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAllocationRoutine);
+    void MapPages(BinaryNinja::Ref<BinaryNinja::BinaryView> dscView, uint64_t sessionID, size_t vm_address, size_t fileoff, size_t size, const std::string& filePath, std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAllocationRoutine);
 
     bool AddressIsMapped(uint64_t address);