From 47538c534fe0aadc626c09d121773fee8ea10d71 Mon Sep 17 00:00:00 2001 From: X512 Date: Sun, 5 Oct 2025 15:00:29 +0900 Subject: [PATCH] ClientMemoryAllocator: use map and reference count - Resolve TODO about using map for server area_id lookup. - Remove server cloned area delete request logic because it is less efficient and robust. Cloned area delete request messages may be missed if client message queue is full so it may cause cloned area leak. - Implement reference counting for cloned areas instead. Change-Id: Ie434ad36c2761ab0df00d341d55a6cea67b69830 Reviewed-on: https://review.haiku-os.org/c/haiku/+/9667 Reviewed-by: waddlesplash Tested-by: Commit checker robot --- headers/private/app/ServerMemoryAllocator.h | 17 +++-- headers/private/app/ServerProtocol.h | 7 -- src/kits/app/Application.cpp | 11 --- src/kits/app/ServerMemoryAllocator.cpp | 75 ++++++++------------- src/kits/interface/Bitmap.cpp | 38 +++++------ src/servers/app/BitmapManager.cpp | 10 ++- src/servers/app/ClientMemoryAllocator.cpp | 22 ++---- src/servers/app/ClientMemoryAllocator.h | 7 +- src/servers/app/ServerApp.cpp | 15 ----- src/servers/app/ServerApp.h | 1 - 10 files changed, 73 insertions(+), 130 deletions(-) diff --git a/headers/private/app/ServerMemoryAllocator.h b/headers/private/app/ServerMemoryAllocator.h index aea3da0c45..92b3ab5cf2 100644 --- a/headers/private/app/ServerMemoryAllocator.h +++ b/headers/private/app/ServerMemoryAllocator.h @@ -9,13 +9,22 @@ #define SERVER_MEMORY_ALLOCATOR_H +#include + #include -#include namespace BPrivate { +struct area_mapping { + int32 reference_count; + area_id server_area; + area_id local_area; + uint8* local_base; +}; + + class ServerMemoryAllocator { public: ServerMemoryAllocator(); @@ -28,11 +37,9 @@ public: bool readOnly = false); void RemoveArea(area_id serverArea); - status_t AreaAndBaseFor(area_id serverArea, - area_id& area, uint8*& base); - private: - BList fAreas; + std::map + fAreas; }; diff --git a/headers/private/app/ServerProtocol.h b/headers/private/app/ServerProtocol.h index 1592ea4716..07e511066d 100644 --- a/headers/private/app/ServerProtocol.h +++ b/headers/private/app/ServerProtocol.h @@ -377,18 +377,11 @@ enum { AS_LAST_CODE }; -// TODO: move this into a private app header, together with the rest of the -// private message definitions in AppDefs.h -enum { - kMsgDeleteServerMemoryArea = '_DSA', -}; - // bitmap allocation flags enum { kAllocator = 0x1, kFramebuffer = 0x2, kHeap = 0x4, - kNewAllocatorArea = 0x8, }; #endif // APP_SERVER_PROTOCOL_H diff --git a/src/kits/app/Application.cpp b/src/kits/app/Application.cpp index c517eb9f64..c5fbfe1911 100644 --- a/src/kits/app/Application.cpp +++ b/src/kits/app/Application.cpp @@ -695,17 +695,6 @@ BApplication::MessageReceived(BMessage* message) _ReconnectToServer(); break; - case kMsgDeleteServerMemoryArea: - { - int32 serverArea; - if (message->FindInt32("server area", &serverArea) == B_OK) { - // The link is not used, but we currently borrow its lock - BPrivate::AppServerLink link; - fServerAllocator->RemoveArea(serverArea); - } - break; - } - default: BLooper::MessageReceived(message); } diff --git a/src/kits/app/ServerMemoryAllocator.cpp b/src/kits/app/ServerMemoryAllocator.cpp index d905cb66f4..65b9fd0f28 100644 --- a/src/kits/app/ServerMemoryAllocator.cpp +++ b/src/kits/app/ServerMemoryAllocator.cpp @@ -31,27 +31,18 @@ static const size_t kReserveMaxSize = 32 * 1024 * 1024; namespace BPrivate { -struct area_mapping { - area_id server_area; - area_id local_area; - uint8* local_base; -}; - - ServerMemoryAllocator::ServerMemoryAllocator() - : - fAreas(4) { } ServerMemoryAllocator::~ServerMemoryAllocator() { - for (int32 i = fAreas.CountItems(); i-- > 0;) { - area_mapping* mapping = (area_mapping*)fAreas.ItemAt(i); - - delete_area(mapping->local_area); - delete mapping; + while (!fAreas.empty()) { + std::map::iterator it = fAreas.begin(); + area_mapping& mapping = it->second; + delete_area(mapping.local_area); + fAreas.erase(it); } } @@ -67,11 +58,24 @@ status_t ServerMemoryAllocator::AddArea(area_id serverArea, area_id& _area, uint8*& _base, size_t size, bool readOnly) { - area_mapping* mapping = new (std::nothrow) area_mapping; - if (mapping == NULL || !fAreas.AddItem(mapping)) { - delete mapping; + std::map::iterator it = fAreas.find(serverArea); + if (it != fAreas.end()) { + area_mapping& mapping = it->second; + mapping.reference_count++; + + _area = mapping.local_area; + _base = mapping.local_base; + + return B_OK; + } + + area_mapping* mapping; + try { + mapping = &fAreas[serverArea]; + } catch (const std::bad_alloc&) { return B_NO_MEMORY; } + mapping->reference_count = 1; status_t status = B_ERROR; uint32 addressSpec = B_ANY_ADDRESS; @@ -95,8 +99,7 @@ ServerMemoryAllocator::AddArea(area_id serverArea, area_id& _area, if (mapping->local_area < B_OK) { status = mapping->local_area; - fAreas.RemoveItem(mapping); - delete mapping; + fAreas.erase(serverArea); return status; } @@ -114,37 +117,15 @@ ServerMemoryAllocator::AddArea(area_id serverArea, area_id& _area, void ServerMemoryAllocator::RemoveArea(area_id serverArea) { - for (int32 i = fAreas.CountItems(); i-- > 0;) { - area_mapping* mapping = (area_mapping*)fAreas.ItemAt(i); - - if (mapping->server_area == serverArea) { - // we found the area we should remove - delete_area(mapping->local_area); - delete mapping; - fAreas.RemoveItem(i); - break; + std::map::iterator it = fAreas.find(serverArea); + if (it != fAreas.end()) { + area_mapping& mapping = it->second; + if (mapping.reference_count-- == 1) { + delete_area(mapping.local_area); + fAreas.erase(serverArea); } } } -status_t -ServerMemoryAllocator::AreaAndBaseFor(area_id serverArea, area_id& _area, - uint8*& _base) -{ - // TODO: why not use a map? - for (int32 i = fAreas.CountItems(); i-- > 0;) { - area_mapping* mapping = (area_mapping*)fAreas.ItemAt(i); - - if (mapping->server_area == serverArea) { - _area = mapping->local_area; - _base = mapping->local_base; - return B_OK; - } - } - - return B_ERROR; -} - - } // namespace BPrivate diff --git a/src/kits/interface/Bitmap.cpp b/src/kits/interface/Bitmap.cpp index a78db5ec85..72e6338acd 100644 --- a/src/kits/interface/Bitmap.cpp +++ b/src/kits/interface/Bitmap.cpp @@ -1097,7 +1097,7 @@ BBitmap::_InitObject(BRect bounds, color_space colorSpace, uint32 flags, error = B_NO_MEMORY; } else { BPrivate::AppServerLink link; - + if (area >= B_OK) { // Use area provided by client @@ -1116,11 +1116,11 @@ BBitmap::_InitObject(BRect bounds, color_space colorSpace, uint32 flags, link.Attach(0); link.Attach(area); link.Attach(areaOffset); - + if (link.FlushWithReply(error) == B_OK && error == B_OK) { link.Read(&fServerToken); link.Read(&fServerArea); - + if (fServerArea >= B_OK) { fSize = size; fColorSpace = colorSpace; @@ -1129,7 +1129,7 @@ BBitmap::_InitObject(BRect bounds, color_space colorSpace, uint32 flags, fFlags = flags; fArea = area; fAreaOffset = areaOffset; - + fBasePointer = (uint8*)info.address + areaOffset; } else error = fServerArea; @@ -1155,25 +1155,20 @@ BBitmap::_InitObject(BRect bounds, color_space colorSpace, uint32 flags, // server side success // Get token link.Read(&fServerToken); - + uint8 allocationFlags; link.Read(&allocationFlags); link.Read(&fServerArea); link.Read(&fAreaOffset); - + BPrivate::ServerMemoryAllocator* allocator = BApplication::Private::ServerAllocator(); - - if ((allocationFlags & kNewAllocatorArea) != 0) { - error = allocator->AddArea(fServerArea, fArea, - fBasePointer, size); - } else { - error = allocator->AreaAndBaseFor(fServerArea, fArea, - fBasePointer); - if (error == B_OK) - fBasePointer += fAreaOffset; - } - + + error = allocator->AddArea(fServerArea, fArea, + fBasePointer, size); + if (error == B_OK) + fBasePointer += fAreaOffset; + if ((allocationFlags & kFramebuffer) != 0) { // The base pointer will now point to an overlay_client_data // structure bytes per row might be modified to match @@ -1181,7 +1176,7 @@ BBitmap::_InitObject(BRect bounds, color_space colorSpace, uint32 flags, link.Read(&bytesPerRow); size = bytesPerRow * (bounds.IntegerHeight() + 1); } - + if (fServerArea >= B_OK) { fSize = size; fColorSpace = colorSpace; @@ -1270,7 +1265,12 @@ BBitmap::_CleanUp() link.Attach(fServerToken); link.Flush(); - // The server areas are deleted via kMsgDeleteServerMemoryArea message + if (fServerArea >= B_OK) { + BPrivate::ServerMemoryAllocator* allocator + = BApplication::Private::ServerAllocator(); + + allocator->RemoveArea(fServerArea); + } fArea = -1; fServerToken = -1; diff --git a/src/servers/app/BitmapManager.cpp b/src/servers/app/BitmapManager.cpp index 8aa0a99adf..4a78107d8e 100644 --- a/src/servers/app/BitmapManager.cpp +++ b/src/servers/app/BitmapManager.cpp @@ -121,13 +121,12 @@ BitmapManager::CreateBitmap(ClientMemoryAllocator* allocator, overlayToken); overlay_client_data* clientData = NULL; - bool newArea = false; if (overlay != NULL && overlay->InitCheck() == B_OK) { // allocate client memory to communicate the overlay semaphore // and buffer location to the BBitmap clientData = (overlay_client_data*)bitmap->fClientMemory.Allocate( - allocator, sizeof(overlay_client_data), newArea); + allocator, sizeof(overlay_client_data)); } if (clientData != NULL) { @@ -139,19 +138,18 @@ BitmapManager::CreateBitmap(ClientMemoryAllocator* allocator, buffer = (uint8*)overlay->OverlayBuffer()->buffer; if (_allocationFlags) - *_allocationFlags = kFramebuffer | (newArea ? kNewAllocatorArea : 0); + *_allocationFlags = kFramebuffer; } else delete overlay; } else if (allocator != NULL) { // standard bitmaps - bool newArea; buffer = (uint8*)bitmap->fClientMemory.Allocate(allocator, - bitmap->BitsLength(), newArea); + bitmap->BitsLength()); if (buffer != NULL) { bitmap->fMemory = &bitmap->fClientMemory; if (_allocationFlags) - *_allocationFlags = kAllocator | (newArea ? kNewAllocatorArea : 0); + *_allocationFlags = kAllocator; } } else { // server side only bitmaps diff --git a/src/servers/app/ClientMemoryAllocator.cpp b/src/servers/app/ClientMemoryAllocator.cpp index 182bcf601b..922c1fbd92 100644 --- a/src/servers/app/ClientMemoryAllocator.cpp +++ b/src/servers/app/ClientMemoryAllocator.cpp @@ -64,7 +64,7 @@ ClientMemoryAllocator::~ClientMemoryAllocator() void* -ClientMemoryAllocator::Allocate(size_t size, block** _address, bool& newArea) +ClientMemoryAllocator::Allocate(size_t size, block** _address) { // A detached allocator no longer allows any further allocations if (fApplication == NULL) @@ -86,11 +86,10 @@ ClientMemoryAllocator::Allocate(size_t size, block** _address, bool& newArea) if (best == NULL) { // We didn't find a free block - we need to allocate // another chunk, or resize an existing chunk - best = _AllocateChunk(size, newArea); + best = _AllocateChunk(size); if (best == NULL) return NULL; - } else - newArea = false; + } // We need to split the chunk into two parts: the one to keep // and the one to give away @@ -184,9 +183,6 @@ ClientMemoryAllocator::Free(block* freeBlock) fChunks.Remove(chunk); delete_area(chunk->area); - if (fApplication != NULL) - fApplication->NotifyDeleteClientArea(chunk->area); - free(chunk); } } @@ -227,7 +223,7 @@ ClientMemoryAllocator::Dump() struct block* -ClientMemoryAllocator::_AllocateChunk(size_t size, bool& newArea) +ClientMemoryAllocator::_AllocateChunk(size_t size) { // round up to multiple of page size size = (size + B_PAGE_SIZE - 1) & ~(B_PAGE_SIZE - 1); @@ -238,10 +234,8 @@ ClientMemoryAllocator::_AllocateChunk(size_t size, bool& newArea) struct chunk* chunk; while ((chunk = iterator.Next()) != NULL) { status_t status = resize_area(chunk->area, chunk->size + size); - if (status == B_OK) { - newArea = false; + if (status == B_OK) break; - } } // TODO: resize and relocate while holding the write lock @@ -288,7 +282,6 @@ ClientMemoryAllocator::_AllocateChunk(size_t size, bool& newArea) chunk->size = size; fChunks.Add(chunk); - newArea = true; } else { // create new free block for this chunk block = (struct block *)malloc(sizeof(struct block)); @@ -333,12 +326,11 @@ ClientMemory::~ClientMemory() void* -ClientMemory::Allocate(ClientMemoryAllocator* allocator, size_t size, - bool& newArea) +ClientMemory::Allocate(ClientMemoryAllocator* allocator, size_t size) { fAllocator.SetTo(allocator, false); - return fAllocator->Allocate(size, &fBlock, newArea); + return fAllocator->Allocate(size, &fBlock); } diff --git a/src/servers/app/ClientMemoryAllocator.h b/src/servers/app/ClientMemoryAllocator.h index 799ebfa219..40ffbf7747 100644 --- a/src/servers/app/ClientMemoryAllocator.h +++ b/src/servers/app/ClientMemoryAllocator.h @@ -40,8 +40,7 @@ public: ClientMemoryAllocator(ServerApp* application); ~ClientMemoryAllocator(); - void* Allocate(size_t size, block** _address, - bool& newArea); + void* Allocate(size_t size, block** _address); void Free(block* cookie); void Detach(); @@ -49,7 +48,7 @@ public: void Dump(); private: - struct block* _AllocateChunk(size_t size, bool& newArea); + struct block* _AllocateChunk(size_t size); private: ServerApp* fApplication; @@ -76,7 +75,7 @@ public: virtual ~ClientMemory(); void* Allocate(ClientMemoryAllocator* allocator, - size_t size, bool& newArea); + size_t size); virtual area_id Area(); virtual uint8* Address(); diff --git a/src/servers/app/ServerApp.cpp b/src/servers/app/ServerApp.cpp index f345d9bf99..530ce750fc 100644 --- a/src/servers/app/ServerApp.cpp +++ b/src/servers/app/ServerApp.cpp @@ -473,21 +473,6 @@ ServerApp::RemovePicture(ServerPicture* picture) } -/*! Called from the ClientMemoryAllocator whenever a server area could be - deleted. - A message is then sent to the client telling it that it can delete its - client area, too. -*/ -void -ServerApp::NotifyDeleteClientArea(area_id serverArea) -{ - BMessage notify(kMsgDeleteServerMemoryArea); - notify.AddInt32("server area", serverArea); - - SendMessageToClient(¬ify); -} - - /*! \brief Send a message to the ServerApp's BApplication \param message The message to send */ diff --git a/src/servers/app/ServerApp.h b/src/servers/app/ServerApp.h index c21869d7d8..7376d9d429 100644 --- a/src/servers/app/ServerApp.h +++ b/src/servers/app/ServerApp.h @@ -96,7 +96,6 @@ public: BPrivate::BTokenSpace& ViewTokens() { return fViewTokens; } - void NotifyDeleteClientArea(area_id serverArea); AppFontManager* FontManager() { return fAppFontManager; } private: