Skip to content

Commit b14f86d

Browse files
bdash0cyn
bdash
authored and
0cyn
committed
[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.
1 parent 0cea933 commit b14f86d

File tree

3 files changed

+72
-57
lines changed

3 files changed

+72
-57
lines changed

‎view/sharedcache/core/SharedCache.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -780,7 +780,7 @@ void SharedCache::PerformInitialLoad()
780780
if (imageHeader->linkeditPresent && vm->AddressIsMapped(imageHeader->linkeditSegment.vmaddr))
781781
{
782782
auto mapping = vm->MappingAtAddress(imageHeader->linkeditSegment.vmaddr);
783-
imageHeader->exportTriePath = mapping.first.filePath;
783+
imageHeader->exportTriePath = mapping.first.fileAccessor->filePath();
784784
}
785785
MutableState().headers[start.second] = imageHeader.value();
786786
CacheImage image;
@@ -3330,7 +3330,8 @@ extern "C"
33303330
images[i].mappings[j].vmAddress = sectionStart;
33313331
images[i].mappings[j].size = header.sections[j].size;
33323332
images[i].mappings[j].name = BNAllocString(header.sectionNames[j].c_str());
3333-
images[i].mappings[j].filePath = BNAllocString(vm->MappingAtAddress(sectionStart).first.filePath.c_str());
3333+
auto fileAccessor = vm->MappingAtAddress(sectionStart).first.fileAccessor;
3334+
images[i].mappings[j].filePath = BNAllocStringWithLength(fileAccessor->filePath().data(), fileAccessor->filePath().length());
33343335
images[i].mappings[j].loaded = cache->object->IsMemoryMapped(sectionStart);
33353336
}
33363337
i++;

‎view/sharedcache/core/VM.cpp

Lines changed: 50 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -203,55 +203,57 @@ void MMAP::Unmap()
203203
}
204204

205205

206-
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)
206+
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)
207207
{
208208
std::scoped_lock<std::mutex> lock(fileAccessorsMutex);
209-
if (fileAccessors.count(path) == 0)
210-
{
211-
auto fileAcccessor = std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>>(new SelfAllocatingWeakPtr<MMappedFileAccessor>(
212-
// Allocator logic for the SelfAllocatingWeakPtr
213-
[path=path, sessionID=sessionID, dscView](){
214-
std::unique_lock<std::mutex> _lock(fileAccessorDequeMutex);
215-
216-
// Iterate through held references and start removing them until we can get a file pointer
217-
// FIXME: This could clear all currently used file pointers and still not get one. FIX!
218-
// We should probably use a condition variable here to wait for a file pointer to be released!!!
219-
for (auto& [_, fileAccessorDeque] : fileAccessorReferenceHolder)
220-
{
221-
if (fileAccessorSemaphore.try_acquire())
222-
break;
223-
fileAccessorDeque.pop_front();
224-
}
225-
226-
mmapCount++;
227-
_lock.unlock();
228-
auto accessor = std::shared_ptr<MMappedFileAccessor>(new MMappedFileAccessor(ResolveFilePath(dscView, path)), [](MMappedFileAccessor* accessor){
229-
// worker thread or we can deadlock on exit here.
230-
BinaryNinja::WorkerEnqueue([accessor](){
231-
fileAccessorSemaphore.release();
232-
mmapCount--;
233-
if (fileAccessors.count(accessor->m_path))
234-
{
235-
std::scoped_lock<std::mutex> lock(fileAccessorsMutex);
236-
fileAccessors.erase(accessor->m_path);
237-
}
238-
delete accessor;
239-
}, "MMappedFileAccessor Destructor");
240-
});
241-
_lock.lock();
242-
// If some background thread has managed to try and open a file when the BV was already closed,
243-
// we can still give them the file they want so they dont crash, but as soon as they let go it's gone.
244-
if (!blockedSessionIDs.count(sessionID))
245-
fileAccessorReferenceHolder[sessionID].push_back(accessor);
246-
return accessor;
247-
},
248-
[postAllocationRoutine=postAllocationRoutine](std::shared_ptr<MMappedFileAccessor> accessor){
249-
if (postAllocationRoutine)
250-
postAllocationRoutine(accessor);
251-
}));
252-
fileAccessors.insert_or_assign(path, fileAcccessor);
209+
if (auto it = fileAccessors.find(path); it != fileAccessors.end()) {
210+
return it->second;
253211
}
254-
return fileAccessors.at(path);
212+
213+
auto fileAcccessor = std::make_shared<LazyMappedFileAccessor>(
214+
path,
215+
// Allocator logic for the SelfAllocatingWeakPtr
216+
[path=path, sessionID=sessionID, dscView](){
217+
std::unique_lock<std::mutex> _lock(fileAccessorDequeMutex);
218+
219+
// Iterate through held references and start removing them until we can get a file pointer
220+
// FIXME: This could clear all currently used file pointers and still not get one. FIX!
221+
// We should probably use a condition variable here to wait for a file pointer to be released!!!
222+
for (auto& [_, fileAccessorDeque] : fileAccessorReferenceHolder)
223+
{
224+
if (fileAccessorSemaphore.try_acquire())
225+
break;
226+
fileAccessorDeque.pop_front();
227+
}
228+
229+
mmapCount++;
230+
_lock.unlock();
231+
auto accessor = std::shared_ptr<MMappedFileAccessor>(new MMappedFileAccessor(ResolveFilePath(dscView, path)), [](MMappedFileAccessor* accessor){
232+
// worker thread or we can deadlock on exit here.
233+
BinaryNinja::WorkerEnqueue([accessor](){
234+
fileAccessorSemaphore.release();
235+
mmapCount--;
236+
if (fileAccessors.count(accessor->m_path))
237+
{
238+
std::scoped_lock<std::mutex> lock(fileAccessorsMutex);
239+
fileAccessors.erase(accessor->m_path);
240+
}
241+
delete accessor;
242+
}, "MMappedFileAccessor Destructor");
243+
});
244+
_lock.lock();
245+
// If some background thread has managed to try and open a file when the BV was already closed,
246+
// we can still give them the file they want so they dont crash, but as soon as they let go it's gone.
247+
if (!blockedSessionIDs.count(sessionID))
248+
fileAccessorReferenceHolder[sessionID].push_back(accessor);
249+
return accessor;
250+
},
251+
[postAllocationRoutine=postAllocationRoutine](std::shared_ptr<MMappedFileAccessor> accessor){
252+
if (postAllocationRoutine)
253+
postAllocationRoutine(std::move(accessor));
254+
});
255+
fileAccessors.insert_or_assign(path, fileAcccessor);
256+
return fileAcccessor;
255257
}
256258

257259

@@ -482,7 +484,7 @@ VM::~VM()
482484
}
483485

484486

485-
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)
487+
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)
486488
{
487489
// The mappings provided for shared caches will always be page aligned.
488490
// 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
495497
}
496498

497499
auto accessor = MMappedFileAccessor::Open(std::move(dscView), sessionID, filePath, postAllocationRoutine);
498-
auto [it, inserted] = m_map.insert_or_assign({vm_address, vm_address + size}, PageMapping(std::move(filePath), std::move(accessor), fileoff));
500+
auto [it, inserted] = m_map.insert_or_assign({vm_address, vm_address + size}, PageMapping(std::move(accessor), fileoff));
499501
if (m_safe && !inserted)
500502
{
501503
BNLogWarn("Remapping page 0x%zx (f: 0x%zx)", vm_address, fileoff);

‎view/sharedcache/core/VM.h

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,25 @@ class MMAP {
104104
void Unmap();
105105
};
106106

107+
class LazyMappedFileAccessor : public SelfAllocatingWeakPtr<MMappedFileAccessor> {
108+
public:
109+
LazyMappedFileAccessor(std::string filePath, std::function<std::shared_ptr<MMappedFileAccessor>()> allocator,
110+
std::function<void(std::shared_ptr<MMappedFileAccessor>)> postAlloc)
111+
: SelfAllocatingWeakPtr(std::move(allocator), std::move(postAlloc)), m_filePath(std::move(filePath)) {
112+
}
113+
114+
std::string_view filePath() const { return m_filePath; }
115+
116+
private:
117+
std::string m_filePath;
118+
};
119+
107120
static uint64_t maxFPLimit;
108121
static std::mutex fileAccessorDequeMutex;
109122
static std::unordered_map<uint64_t, std::deque<std::shared_ptr<MMappedFileAccessor>>> fileAccessorReferenceHolder;
110123
static std::set<uint64_t> blockedSessionIDs;
111124
static std::mutex fileAccessorsMutex;
112-
static std::unordered_map<std::string, std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>>> fileAccessors;
125+
static std::unordered_map<std::string, std::shared_ptr<LazyMappedFileAccessor>> fileAccessors;
113126
static counting_semaphore fileAccessorSemaphore(0);
114127

115128
static std::atomic<uint64_t> mmapCount = 0;
@@ -123,7 +136,7 @@ class MMappedFileAccessor {
123136
MMappedFileAccessor(const std::string &path);
124137
~MMappedFileAccessor();
125138

126-
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);
139+
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);
127140

128141
static void CloseAll(const uint64_t sessionID);
129142

@@ -179,11 +192,10 @@ class MMappedFileAccessor {
179192

180193

181194
struct PageMapping {
182-
std::string filePath;
183-
std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>> fileAccessor;
195+
std::shared_ptr<LazyMappedFileAccessor> fileAccessor;
184196
size_t fileOffset;
185-
PageMapping(std::string filePath, std::shared_ptr<SelfAllocatingWeakPtr<MMappedFileAccessor>> fileAccessor, size_t fileOffset)
186-
: filePath(std::move(filePath)), fileAccessor(std::move(fileAccessor)), fileOffset(fileOffset) {}
197+
PageMapping(std::shared_ptr<LazyMappedFileAccessor> fileAccessor, size_t fileOffset)
198+
: fileAccessor(std::move(fileAccessor)), fileOffset(fileOffset) {}
187199
};
188200

189201

@@ -249,7 +261,7 @@ class VM {
249261

250262
~VM();
251263

252-
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);
264+
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);
253265

254266
bool AddressIsMapped(uint64_t address);
255267

0 commit comments

Comments
 (0)