From b14f86d5373eb7a4a8a40c9a23c3a9faf2732eeb Mon Sep 17 00:00:00 2001 From: Mark Rowe Date: Mon, 13 Jan 2025 22:19:17 -0800 Subject: [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. --- view/sharedcache/core/SharedCache.cpp | 5 +- view/sharedcache/core/VM.cpp | 98 ++++++++++++++++++----------------- view/sharedcache/core/VM.h | 26 +++++++--- 3 files changed, 72 insertions(+), 57 deletions(-) (limited to 'view/sharedcache') 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> MMappedFileAccessor::Open(BinaryNinja::Ref dscView, const uint64_t sessionID, const std::string &path, std::function)> postAllocationRoutine) +std::shared_ptr MMappedFileAccessor::Open(BinaryNinja::Ref dscView, const uint64_t sessionID, const std::string &path, std::function)> postAllocationRoutine) { std::scoped_lock lock(fileAccessorsMutex); - if (fileAccessors.count(path) == 0) - { - auto fileAcccessor = std::shared_ptr>(new SelfAllocatingWeakPtr( - // Allocator logic for the SelfAllocatingWeakPtr - [path=path, sessionID=sessionID, dscView](){ - std::unique_lock _lock(fileAccessorDequeMutex); - - // 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(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 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 accessor){ - if (postAllocationRoutine) - postAllocationRoutine(accessor); - })); - fileAccessors.insert_or_assign(path, fileAcccessor); + if (auto it = fileAccessors.find(path); it != fileAccessors.end()) { + return it->second; } - return fileAccessors.at(path); + + auto fileAcccessor = std::make_shared( + path, + // Allocator logic for the SelfAllocatingWeakPtr + [path=path, sessionID=sessionID, dscView](){ + std::unique_lock _lock(fileAccessorDequeMutex); + + // 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(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 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 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 dscView, uint64_t sessionID, size_t vm_address, size_t fileoff, size_t size, std::string filePath, std::function)> postAllocationRoutine) +void VM::MapPages(BinaryNinja::Ref dscView, uint64_t sessionID, size_t vm_address, size_t fileoff, size_t size, const std::string& filePath, std::function)> 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 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 { +public: + LazyMappedFileAccessor(std::string filePath, std::function()> allocator, + std::function)> 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>> fileAccessorReferenceHolder; static std::set blockedSessionIDs; static std::mutex fileAccessorsMutex; -static std::unordered_map>> fileAccessors; +static std::unordered_map> fileAccessors; static counting_semaphore fileAccessorSemaphore(0); static std::atomic mmapCount = 0; @@ -123,7 +136,7 @@ public: MMappedFileAccessor(const std::string &path); ~MMappedFileAccessor(); - static std::shared_ptr> Open(BinaryNinja::Ref dscView, const uint64_t sessionID, const std::string &path, std::function)> postAllocationRoutine = nullptr); + static std::shared_ptr Open(BinaryNinja::Ref dscView, const uint64_t sessionID, const std::string &path, std::function)> postAllocationRoutine = nullptr); static void CloseAll(const uint64_t sessionID); @@ -179,11 +192,10 @@ public: struct PageMapping { - std::string filePath; - std::shared_ptr> fileAccessor; + std::shared_ptr fileAccessor; size_t fileOffset; - PageMapping(std::string filePath, std::shared_ptr> fileAccessor, size_t fileOffset) - : filePath(std::move(filePath)), fileAccessor(std::move(fileAccessor)), fileOffset(fileOffset) {} + PageMapping(std::shared_ptr fileAccessor, size_t fileOffset) + : fileAccessor(std::move(fileAccessor)), fileOffset(fileOffset) {} }; @@ -249,7 +261,7 @@ public: ~VM(); - void MapPages(BinaryNinja::Ref dscView, uint64_t sessionID, size_t vm_address, size_t fileoff, size_t size, std::string filePath, std::function)> postAllocationRoutine); + void MapPages(BinaryNinja::Ref dscView, uint64_t sessionID, size_t vm_address, size_t fileoff, size_t size, const std::string& filePath, std::function)> postAllocationRoutine); bool AddressIsMapped(uint64_t address); -- cgit v1.3.1