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 <[email protected]>
Tested-by: Commit checker robot <[email protected]>

headers/private/app/ServerMemoryAllocator.h

@@ -9,13 +9,22 @@
 #define SERVER_MEMORY_ALLOCATOR_H
 
 
+#include <map>
+
 #include <OS.h>
-#include <List.h>
 
 
 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<area_id, area_mapping>
+								fAreas;
 };
 
 

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

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);
 	}

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<area_id, area_mapping>::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<area_id, area_mapping>::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<area_id, area_mapping>::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

src/kits/interface/Bitmap.cpp

@@ -1164,15 +1164,10 @@ BBitmap::_InitObject(BRect bounds, color_space colorSpace, uint32 flags,
 					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
@@ -1270,7 +1265,12 @@ BBitmap::_CleanUp()
 		link.Attach<int32>(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;

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

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);
 }
 
 

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();

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(&notify);
-}
-
-
 /*!	\brief Send a message to the ServerApp's BApplication
 	\param message The message to send
 */

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: