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/VM.cpp | 98 ++++++++++++++++++++++---------------------- 1 file changed, 50 insertions(+), 48 deletions(-) (limited to 'view/sharedcache/core/VM.cpp') 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); -- cgit v1.3.1